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

Updated checking of SYSTEM SYNC REPLICA #45648

Merged
merged 18 commits into from Feb 9, 2023

Conversation

SmitaRKulkarni
Copy link
Member

@SmitaRKulkarni SmitaRKulkarni commented Jan 26, 2023

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Updated checking of SYSTEM SYNC REPLICA resolves #45508
Implementation:

  • Updated to wait for current last entry to be processed (after pulling shared log) instead of queue size becoming 0.
  • Updated Subscriber to notify both queue size and removed log_entry_id.

Implementation:
* Updated to wait for current last entry to be processed (after pulling shared log) instead of queue size becoming 0.
* Updated Subscriber to notify both queue size and removed log_entry_id.
@SmitaRKulkarni SmitaRKulkarni marked this pull request as draft January 26, 2023 10:42
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-improvement Pull request with some product improvements label Jan 26, 2023
@SmitaRKulkarni SmitaRKulkarni marked this pull request as ready for review January 26, 2023 12:55
@alesapin alesapin self-assigned this Jan 26, 2023
@SmitaRKulkarni SmitaRKulkarni marked this pull request as draft January 27, 2023 07:13
@SmitaRKulkarni SmitaRKulkarni marked this pull request as ready for review January 30, 2023 13:55
Copy link
Member

@alesapin alesapin left a comment

Choose a reason for hiding this comment

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

Let's fix callback logic, in rest it's alright.

src/Storages/StorageReplicatedMergeTree.cpp Outdated Show resolved Hide resolved
src/Storages/StorageReplicatedMergeTree.cpp Outdated Show resolved Hide resolved
src/Storages/StorageReplicatedMergeTree.cpp Outdated Show resolved Hide resolved
src/Storages/MergeTree/ReplicatedMergeTreeQueue.h Outdated Show resolved Hide resolved
@SmitaRKulkarni SmitaRKulkarni marked this pull request as draft February 7, 2023 15:41
@SmitaRKulkarni SmitaRKulkarni marked this pull request as ready for review February 7, 2023 20:51
@SmitaRKulkarni
Copy link
Member Author

SmitaRKulkarni commented Feb 8, 2023

@alesapin : log_entry_id was not a good choice for checking if an entry is removed or not, for most of the entries it's empty. I think znode_name is a better option, updated to use that. Let me know if you see any concerns using this or if you have better suggestions.

@@ -544,8 +544,7 @@ void ReplicatedMergeTreeQueue::removeProcessedEntry(zkutil::ZooKeeperPtr zookeep
if (!found && need_remove_from_zk)
throw Exception(ErrorCodes::LOGICAL_ERROR, "Can't find {} in the memory queue. It is a bug. Entry: {}",
entry->znode_name, entry->toString());

notifySubscribers(queue_size);
notifySubscribers(queue_size, entry->znode_name);
Copy link
Member

Choose a reason for hiding this comment

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

Good, of course you are right. Sometimes replica can decide to push something into own queue without log.

@alesapin
Copy link
Member

alesapin commented Feb 9, 2023

Failure unrelated.

@alesapin alesapin merged commit 859f528 into master Feb 9, 2023
@alesapin alesapin deleted the 45508_Update_strategy_for_system_sync_replica branch February 9, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SYSTEM SYNC REPLICA works strange
3 participants