vtorc: add context.Context to more logic, general cleanup
#17903
+180
−191
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timvaillancourt
added
Type: Internal Cleanup
Component: VTorc
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
|
vitess-bot
bot
added
NeedsBackportReason
timvaillancourt
removed
NeedsDescriptionUpdate
timvaillancourt
commented
Mar 5, 2025
| ts = topo.Open() | ||
| tmc = inst.InitializeTMC() | ||
| // Clear existing cache and perform a new refresh. | ||
| if _, err := db.ExecVTOrc("DELETE FROM vitess_tablet"); err != nil { | ||
| if err := inst.DeleteAllTablets(); err != nil { |
There was a problem hiding this comment.
DELETE FROM should be in inst DAO code
timvaillancourt
commented
Mar 5, 2025
| ) | ||
|
|
||
| // ErrTabletAliasNil is a fixed error message. | ||
| var ErrTabletAliasNil = errors.New("tablet alias is nil") | ||
| var tmc tmclient.TabletManagerClient |
There was a problem hiding this comment.
TMClient stuff isn't really DAO code
timvaillancourt
commented
Mar 5, 2025
| @@ -711,38 +692,17 @@ func ReadInstancesWithErrantGTIds(keyspace string, shard string) ([]*Instance, e | |||
| return readInstancesByCondition(condition, args, "") | |||
| } | |||
|
|
|||
| // GetKeyspaceShardName gets the keyspace shard name for the given instance key | |||
| func GetKeyspaceShardName(tabletAlias string) (keyspace string, shard string, err error) { | |||
There was a problem hiding this comment.
inst.ReadTablet (vs .GetKeyspaceShardName) will get you keyspace/shard from the same table, so let's just use that 🤷
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
timvaillancourt
force-pushed
the
vtorc-more-code-cleanup
branch
from
5c86191 to
750567b
Compare
timvaillancourt
commented
Mar 5, 2025
| @@ -36,7 +36,7 @@ const ( | |||
| DiscoveryQueueCapacity = 100000 | |||
| DiscoveryQueueMaxStatisticsSize = 120 | |||
| DiscoveryCollectionRetentionSeconds = 120 | |||
| UnseenInstanceForgetHours = 240 // Number of hours after which an unseen instance is forgotten | |||
| UnseenInstanceForgetHours = 12 // Number of hours after which an unseen instance is forgotten | |||
There was a problem hiding this comment.
1/2 a day unseen feels like long enough
timvaillancourt
requested review from
deepthi,
shlomi-noach,
GuptaManan100 and
mattlord
as code owners
timvaillancourt
commented
Mar 5, 2025
| defer func() { | ||
| if r := recover(); r != nil { | ||
| err = logReadTopologyInstanceError(tabletAlias, "Unexpected, aborting", tb.Errorf("%+v", r)) | ||
| } | ||
| }() | ||
|
|
||
| var waitGroup sync.WaitGroup |
There was a problem hiding this comment.
This waitGroup is never used aside from a waitGroup.Wait() call
There is never a waitGroup.Add(n) or .Done() 🤷
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17903 +/- ##
==========================================
- Coverage 67.52% 67.51% -0.01%
==========================================
Files 1594 1596 +2
Lines 259564 259660 +96
==========================================
+ Hits 175266 175317 +51
- Misses 84298 84343 +45 ☔ View full report in Codecov by Sentry. |
|
Will 👀 into the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR:
context.Contextto morego/vt/vtorc/instandlogicpackages and passes a cancelable context fromcli.godownUnseenInstanceForgetHoursfrom240->12- 12 hours is a long time to be unseeninst.GetKeyspaceShardNamewithinst.ReadTablet(deletes former)inst.ReadOutdatedInstanceKeys->inst.ReadOutdatedInstancesand acceptfuncvs return slice*_dao.go-> new/existing filesRelated Issue(s)
#17330
Checklist
Deployment Notes