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

SAI-5162: Experimental downnode approach to use CoreContainer if available #232

Draft
wants to merge 3 commits into
base: fs/branch_9x
Choose a base branch
from

Conversation

patsonluk
Copy link
Collaborator

@patsonluk patsonluk commented Oct 11, 2024

Description

As introduced in this fix, downnode calls will now discover replicas on a node via zk which is more reliable then CoreContainer, which might not have the full picture of the replicas if it's not loaded yet (during startup). This however, might impose some extra overhead for big cluster with alot of collections, especially when a large number of collections do NOT have actual replica on the target node, since those are "lazy" collection ref that has no watch which might require a full collection state fetch (exists, and fetch from ZK).

A detailed breakdown of the overhead changes with such fix:

Node shutdown

Before fix : No ZK fetch as it gets list of replicas from CoreContainer which only contains replicas this node hosts. Therefore they are watched already (no lazy collection ref).
After fix: No ZK fetch on collections that have replica on this node since they are watched by the ZkStateReader, however other collections will trigger full ZK fetch on state

Node startup

Before fix: No ZK fetch, as the downnode has no effect due to the bug
After fix: Zk fetch on all collections since none of them is being watched yet (core was not loaded/registered to the container yet)

Solution

This is purely just an experiment to understand the scope of code change required to implement the approach to use core container if possible . We might not want such code complexity if the new overhead is not great enough to justify the change!

In theory, we can still use the CoreContainer approach for non-startup. Factored out a new method getReplicasPerCollectionOnThisNode to achieve that

Remarks

Take note that this PR also slightly optimizes the existing flow of reading from ZK. Since the existing code actually does a full fetch once and then does another exists check (docCollection is obtained twice), while this PR fetches doCollection only once.

Though this likely makes little difference as it only saves an exists call, which is fast.

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.

1 participant