Skip to content

feat: show repartitioning info in validate-restore report#43

Open
thomasdudek wants to merge 2 commits intoosodevops:mainfrom
Beyond-Bits:feat/validate-restore-repartitioning-info
Open

feat: show repartitioning info in validate-restore report#43
thomasdudek wants to merge 2 commits intoosodevops:mainfrom
Beyond-Bits:feat/validate-restore-repartitioning-info

Conversation

@thomasdudek
Copy link
Contributor

Display auto-repartitioning details (strategy, source/target partition counts) in the dry-run validation report when repartitioning is configured for a topic. Also rewrites the CLI report printer to use dynamic box width with proper alignment.

  • Add DryRunRepartitioningInfo struct to manifest types
  • Populate repartitioning info from restore config in dry_run()
  • Implement Display for RepartitioningStrategy
  • Rewrite print_validation_report with dynamic-width box rendering

@thomasdudek thomasdudek force-pushed the feat/validate-restore-repartitioning-info branch from 5b574cd to 9c8ec36 Compare March 4, 2026 21:22
Display auto-repartitioning details (strategy, source/target partition
counts) in the dry-run validation report when repartitioning is
configured for a topic. Also rewrites the CLI report printer to use
dynamic box width with proper alignment.

- Add DryRunRepartitioningInfo struct to manifest types
- Populate repartitioning info from restore config in dry_run()
- Implement Display for RepartitioningStrategy
- Rewrite print_validation_report with dynamic-width box rendering

Co-authored-by: Claude <noreply@anthropic.com>
@thomasdudek thomasdudek force-pushed the feat/validate-restore-repartitioning-info branch from 9c8ec36 to 884ef22 Compare March 4, 2026 21:50
Copy link
Contributor

@sionsmith sionsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Looks good — approve with minor suggestions

Build verification ✅

  • cargo build — passes
  • cargo test — 89 tests pass
  • cargo clippy — clean

What works well ✅

  1. Repartitioning info in dry-run — clean addition. DryRunRepartitioningInfo struct is well-placed, serde(skip_serializing_if) keeps JSON output clean, and the Display impl on RepartitioningStrategy is reusable.

  2. Dynamic box width — big improvement over the old hardcoded-width approach, which had visible alignment issues (the original lines didn't even align — compare the header/separator widths in the old code). The new code sizes the box to content correctly.

  3. Repartitioning config lookup is correct — uses get(&target_topic) which matches the config convention (keyed by target topic name per config.rs:642).

  4. #[non_exhaustive] on DryRunTopicReport — good forward-thinking. Since the only construction site is within kafka-backup-core (same crate), this has no current impact but protects against downstream breakage if the struct gains more fields.

  5. Correct mapping of source_partitions — uses topic_backup.partitions.len() (actual source partition count from the backup manifest) rather than a config value.

Minor suggestions (non-blocking)

  1. \x01 sentinel for centered lines — using a control character as an in-band signal works but is unconventional. A small enum like enum Line { Centered(String), Left(String) } or a Vec<(String, bool)> would be more self-documenting. Not a blocker since it's confined to one function.

  2. Unicode display widthchars().count() counts code points, not terminal display columns. Characters like , , may render as 1 or 2 columns wide depending on the terminal, causing slight box misalignment. A crate like unicode-width would fix this, but it's a pre-existing issue from the old code and only cosmetic. Not worth adding a dependency for.

  3. Empty separator lines in topics sectionlines.push(String::new()) at line 107 renders as a blank row inside the box (║ ║). This is fine aesthetically but slightly unusual. Consider if a visual separator like "─".repeat(...) or just omitting it would look better.

No issues found ✅

No bugs, no correctness problems, no breaking changes. Clean feature addition.

Copy link
Contributor

@sionsmith sionsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failure: #[non_exhaustive] breaks semver check

The Detect Breaking Changes check is failing because #[non_exhaustive] was added to DryRunTopicReport. cargo-semver-checks correctly flags this — adding #[non_exhaustive] to a previously exhaustive public struct is a breaking change (it prevents struct literal construction from outside the crate).

--- failure struct_marked_non_exhaustive: struct marked #[non_exhaustive] ---
  struct DryRunTopicReport in manifest.rs:843

Fix

Remove #[non_exhaustive] from DryRunTopicReport in crates/kafka-backup-core/src/manifest.rs:842:

 /// Per-topic dry-run report
 #[derive(Debug, Clone, Serialize, Deserialize)]
-#[non_exhaustive]
 pub struct DryRunTopicReport {

It's not needed here — the new repartitioning: Option<DryRunRepartitioningInfo> field already handles forward compatibility naturally (None default, serde skips on serialization). Everything else in the PR looks good.

Changing review from approve to request-changes since CI is blocked on this.

@thomasdudek
Copy link
Contributor Author

Thanks for the thorough review!

Addressed suggestions

\x01 sentinel replaced with enum — Agreed this was unconventional. Replaced with a BoxLine enum (Left(String), Centered(String), Separator) which makes the intent self-documenting.

Empty separator lines — Replaced lines.push(String::new()) (blank row) with a BoxLine::Separator variant that renders a line between topics. Also changed it to only appear between topics (not before the first one).

Unicode display width — Acknowledged as a pre-existing cosmetic issue. Agreed it's not worth adding a dependency for.

Regarding #[non_exhaustive] and semver CI

The semver check fails either way:

  • With #[non_exhaustive]: struct_marked_non_exhaustive — adding the attribute to a previously exhaustive struct is a breaking change.
  • Without #[non_exhaustive]: constructible_struct_adds_field — adding the repartitioning field to a constructible struct is also a breaking change.

Both are technically semver-major because external code could construct DryRunTopicReport via struct literal. The underlying issue is that adding any new field to a public struct without #[non_exhaustive] is a breaking change per semver rules.

Options:

  1. Bump to 1.0.0 (or next major) — correct per semver, but seems premature for a minor feature addition.
  2. Add #[non_exhaustive] to all public structs in manifest.rs as a separate semver-major release first, then this PR becomes a clean minor addition on top.

What's your preference? Or do I miss some other option?

@sionsmith
Copy link
Contributor

Thanks for addressing the \x01 sentinel — the BoxLine enum is much cleaner!

The Detect Breaking Changes check is still failing though — #[non_exhaustive] on DryRunTopicReport is still present in manifest.rs:842. That's the only thing blocking CI. Just remove that one line and it should go green.

Address PR review feedback:
- Replace control character sentinel with a self-documenting BoxLine enum
  (Left, Centered, Separator variants)
- Replace empty line between topics with a visual ─ separator line
- Only show separator between topics, not before the first one

Co-authored-by: Claude <noreply@anthropic.com>
@thomasdudek thomasdudek force-pushed the feat/validate-restore-repartitioning-info branch from 7225e52 to 9bf2f9f Compare March 6, 2026 07:43
@thomasdudek
Copy link
Contributor Author

thomasdudek commented Mar 6, 2026

The Detect Breaking Changes check is still failing though — #[non_exhaustive] on DryRunTopicReport is still present in manifest.rs:842. That's the only thing blocking CI. Just remove that one line and it should go green.

I removed it, but like mentioned in the comment above, this unfortunately brings up an other semver issue:

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/constructible_struct_adds_field.ron

this at least is equal to the one you've had in your feature PR https://github.com/osodevops/kafka-backup/actions/runs/22666759032/job/65700335839#step:4:74

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants