vtexplain: Ensure memory topo is set up for throttler#15279
Merged
dbussink merged 1 commit intovitessio:mainfrom Feb 19, 2024
Merged
vtexplain: Ensure memory topo is set up for throttler#15279dbussink merged 1 commit intovitessio:mainfrom
dbussink merged 1 commit intovitessio:mainfrom
Conversation
dbussink
requested review from
ajm188,
deepthi,
harshit-gangal,
mattlord,
rohit-nayak-ps,
shlomi-noach and
systay
as code owners
Contributor
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
dbussink
commented
Feb 18, 2024
go/vt/vtexplain/vtexplain_vtgate.go
Outdated
Member
Author
There was a problem hiding this comment.
@shlomi-noach In order to not have the throttler log errors inside vtexplain, I had to set up the above minimum stuff in the memory topo for SrvKeyspace.
I wonder if there's a better way for this and if this could be refactored in a better way? Should we have some option to disable more parts of the throttler when we're inside vtexplain?
I guess alternatively, we could make the throttler a parameter in NewTabletServer instead to inject it (which is called inside vtexplain), so we could inject a no-op throttler? Any other ideas?
Contributor
There was a problem hiding this comment.
@dbussink could you please clarify what errors did the throttler log inside vtexplain?
dbussink
force-pushed
the
fix-vtexplain-throttler-setup
branch
from
047c167 to
e35d1a7
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15279 +/- ##
==========================================
+ Coverage 67.41% 67.48% +0.06%
==========================================
Files 1560 1561 +1
Lines 192752 193330 +578
==========================================
+ Hits 129952 130469 +517
- Misses 62800 62861 +61 ☔ View full report in Codecov by Sentry. |
dbussink
force-pushed
the
fix-vtexplain-throttler-setup
branch
from
e35d1a7 to
116a27e
Compare
dbussink
added
Type: Bug
Component: TabletManager
Component: vtexplain
Contributor
shlomi-noach
left a comment
shlomi-noach
left a comment
There was a problem hiding this comment.
Other than the obvious change in throttler.go where we do not log interrupted/cancelled errors, I'm not sure I follow the intent of this PR and what issues are there with the throttler using memorytopo.
go/vt/vtexplain/vtexplain_vtgate.go
Outdated
Contributor
There was a problem hiding this comment.
@dbussink could you please clarify what errors did the throttler log inside vtexplain?
shlomi-noach
approved these changes
Feb 19, 2024
Contributor
shlomi-noach
left a comment
shlomi-noach
left a comment
There was a problem hiding this comment.
For now, this is a good workaround. We need to review how we run tabletserver within the context of vtexplain. We are not interested in the throttler at all running in such context, and possibly other components (heartbeat writer, etc.). We'll take that into a separate discussion/change.
Contributor
|
To be backported to all supported versions as the throttler will produce the same kind of logging on all versions since |
dbussink
force-pushed
the
fix-vtexplain-throttler-setup
branch
from
116a27e to
bd20ce3
Compare
dbussink
commented
Feb 19, 2024
go/vt/srvtopo/watch_srvkeyspace.go
Outdated
Member
Author
There was a problem hiding this comment.
This was incorrectly using context.Background(). I also audited the paths into this function and from things like the CLI invocation, we do pass in the background context correctly.
See also watch_srvvschema where we do have the logic correct.
Member
Author
|
No more stray logging after this change (together with #15275). BeforeAfterNo more noise and also a lot faster test runtime. |
rohit-nayak-ps
approved these changes
Feb 19, 2024
This logic ensures the memorytopo is set up for the throttler logic so that it doesn't log any errors. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
dbussink
force-pushed
the
fix-vtexplain-throttler-setup
branch
from
bd20ce3 to
42e2746
Compare
vitess-bot
pushed a commit
that referenced
this pull request
Feb 19, 2024
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This logic ensures the memorytopo is set up for the throttler logic so that it doesn't log any errors.
Needs to be backported to all supported versions, since the throttler is part of all of those.
Related Issue(s)
Part of #15242
Checklist