Skip to content

Update metadata on replica recovery#23742

Merged
tavplubix merged 3 commits intomasterfrom
update_metadata_on_replica_recovery
May 8, 2021
Merged

Update metadata on replica recovery#23742
tavplubix merged 3 commits intomasterfrom
update_metadata_on_replica_recovery

Conversation

@tavplubix
Copy link
Copy Markdown
Member

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Bug Fix

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed a bug in recovery of staled ReplicatedMergeTree replica. Some metadata updates could be ignored by staled replica if ALTER query was executed during downtime of the replica.

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Apr 28, 2021
@tavplubix tavplubix requested a review from alesapin April 28, 2021 17:56
/// Our metadata it not up to date with source replica metadata.
/// Metadata is updated by ALTER_METADATA entries, but some entries are probably cleaned up from the log.
/// It's also possible that some newer ALTER_METADATA entries are present in source_queue list,
/// and source replica are executing such entry right not (or had executed recently).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/// and source replica are executing such entry right not (or had executed recently).
/// and source replica are executing such entry right now (or had executed recently).

int alter_version = -1; /// May be equal to -1, if it's normal mutation, not metadata update.

/// only ALTER METADATA command
/// TODO Seems like it's never used
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it was a mistake...


zookeeper->create(replica_path + "/queue/queue-", dummy_alter.toString(), zkutil::CreateMode::PersistentSequential);

/// TODO Do we also need to do something with mutation_pointer? Seems like we don't, but it's not clear from the code.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Definitely yes, we shouldn't do anything here, but our mutation pointer must be equal with the source replica's pointer?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, it may be less than mutation pointer of the source replica

@tavplubix tavplubix merged commit f8c7725 into master May 8, 2021
@tavplubix tavplubix deleted the update_metadata_on_replica_recovery branch May 8, 2021 13:34
tavplubix added a commit that referenced this pull request May 9, 2021
Backport #23742 to 21.3: Update metadata on replica recovery
tavplubix added a commit that referenced this pull request May 9, 2021
Backport #23742 to 21.5: Update metadata on replica recovery
tavplubix added a commit that referenced this pull request May 9, 2021
Backport #23742 to 21.4: Update metadata on replica recovery
tavplubix added a commit that referenced this pull request Jun 4, 2021
Backport #23742 to 20.8: Update metadata on replica recovery
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants