Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancements to YBD payload after assessment refactoring #2238

Merged
merged 4 commits into from
Jan 23, 2025

Conversation

sanyamsinghal
Copy link
Collaborator

@sanyamsinghal sanyamsinghal commented Jan 23, 2025

Describe the changes in this pull request

  • Need to add Migration Complexity Explainability to YBD and bump up the payload version
  • Ensure assessment issue struct in ybd and assessment are in sync
  • Bump up the payload version

Describe if there are any user-facing changes

Yes but it is backward compatible.
Also the payload would change for YugabyteD, wrt to some sub-fields in AssessmentIssue field but since they haven't picked up previous payload version, they can just directly jump to implementing this(being more future proof).

How was this pull request tested?

Existing tests.
Manually tested ybd ui is not broken for assessment phase.

Does your PR have changes that can cause upgrade issues?

Component Breaking changes?
MetaDB Yes/No
Name registry json Yes/No
Data File Descriptor Json Yes/No
Export Snapshot Status Json Yes/No
Import Data State Yes/No
Export Status Json Yes/No
Data .sql files of tables Yes/No
Export and import data queue Yes/No
Schema Dump Yes/No
AssessmentDB Yes/No
Sizing DB Yes/No
Migration Assessment Report Json Yes/No
Callhome Json Yes/No
YugabyteD Tables Yes/No
TargetDB Metadata Tables Yes/No

@sanyamsinghal sanyamsinghal self-assigned this Jan 23, 2025
@sanyamsinghal sanyamsinghal marked this pull request as ready for review January 23, 2025 10:56
@sanyamsinghal
Copy link
Collaborator Author

VoyagerVersion string
TargetDBVersion *ybversion.YBVersion
MigrationComplexity string
MigrationComplexityExplanation string
Copy link
Contributor

Choose a reason for hiding this comment

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

How are we populating MigrationComplexityExplanation, don't see any change related to that here?

Copy link
Contributor

Choose a reason for hiding this comment

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

OR are we just adding the field but not sending the actual explanation as of this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh right, that was a miss. Updated.

Copy link
Contributor

@priyanshi-yb priyanshi-yb left a comment

Choose a reason for hiding this comment

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

LGTM!

@sanyamsinghal sanyamsinghal merged commit 6bf1539 into main Jan 23, 2025
68 checks passed
@sanyamsinghal sanyamsinghal deleted the sanyam/ybd-assess branch January 23, 2025 12:00
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