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

refactor(cluster): #2652 initiate migration process from CONFIG cmd #2667

Merged
merged 4 commits into from
Feb 29, 2024

Conversation

BorysTheDev
Copy link
Contributor

No description provided.

@@ -757,12 +770,20 @@ ClusterSlotMigration* ClusterFamily::AddMigration(std::string host_ip, uint16_t
.get();
}

void ClusterFamily::RemoveFinishedIncomingMigrations() {
void ClusterFamily::RemoveFinishedMigrations() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

but now we should remove a migration from the map only if it is not in the config anymore, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I will add this functionality later when remove finalization step

struct ClusterShard {
SlotRanges slot_ranges;
Node master;
std::vector<Node> replicas;
std::vector<MigrationInfo> migrations;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be an optional field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any point to make it optional if it can be empty

Copy link
Collaborator

Choose a reason for hiding this comment

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

all the places that you write .migrations = {} can be removed than?
this is just optional data and I think setting .migrations = {} is redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a compilation warning, because not members are initialized

Copy link
Collaborator

Choose a reason for hiding this comment

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

and if we change it to optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

compiler thinks that we forget about this field when we use this method of initialization

@BorysTheDev BorysTheDev merged commit e57067d into main Feb 29, 2024
10 checks passed
@BorysTheDev BorysTheDev deleted the start_migration_from_config branch February 29, 2024 14:08
romange pushed a commit that referenced this pull request Mar 1, 2024
…2667)

* refactor(cluster): #2652 initiate migration process from CONFIG cmd
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.

None yet

2 participants