-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Change healthcheck to only reload specific keyspace shard tablets #17872
base: main
Are you sure you want to change the base?
Change healthcheck to only reload specific keyspace shard tablets #17872
Conversation
Signed-off-by: Manan Gupta <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Manan Gupta <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17872 +/- ##
==========================================
+ Coverage 67.45% 67.53% +0.07%
==========================================
Files 1594 1595 +1
Lines 259064 259701 +637
==========================================
+ Hits 174760 175377 +617
- Misses 84304 84324 +20 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
// if we get a partial result, we keep going. It most likely means | ||
// a cell is out of commission. | ||
aliases, err := ts.FindAllTabletAliasesInShardByCell(ctx, keyspace, shard, cells) | ||
if err != nil && !IsErrType(err, PartialResult) { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this func make it correct, but slower. Previously the keyspace and shard were being ignored and we'd return all tablets. Where is this func being used, and what is the impact of fetching each tablet from topo versus (in the best case) a single topo call fetching all tablets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is being used only in VTOrc to refresh the tablets in a specific keyspace shard.
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Description
This PR fixes the issue described in #17876. As the issue points out, we should only be loading tablets for the specific keyspace-shard that we don't have a primary for anymore.
This PR makes this change, by changing the load tablets trigger, to also store the keyspace shard information, and we use that to only load some of the tablets.
As part of this change, I also found that as part of the PR #17071 we introduced a couple of fields to hte
GetTabletsByCellOptions
but they weren't being respected in theGetTabletsMap
function. This PR fixes this too, and adds tests for all the topo functions touched. The implementation of a couple of them has also been changed to improve efficiency.Related Issue(s)
Checklist
Deployment Notes