-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Fix query routing for TSDB distinct counter and frequency table delete operations. #5451
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
Conversation
| vnodes=64, | ||
| enable_frequency_sketches=True, | ||
| hosts={ | ||
| i - 6: {'db': i} for i in xrange(6, 9) |
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.
All other tests typically run on db 9 so I just picked some arbitrary numbers around there
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.
arbitrary numbers
🤔
LewisJEllis
left a comment
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.
LGTM
tests/sentry/tsdb/test_redis.py
Outdated
| ) | ||
|
|
||
| def tearDown(self): | ||
| with self.db.cluster.fanout('all') as client: |
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.
I guess docs say it's the same exact thing, but can we do
with self.db.cluster.all() as client:here? Or do we prefer having "fanout" in there to hint at what's going on?
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.
No specific reason to prefer one over the other, I just went with the one that I remembered exists. I changed it to .all because it seems more reliable to use a method here.
This was an oversight as part of GH-5317, and wasn't caught in tests because the test suite didn't actually run the tests against multiple databases. The easiest way to test this (other than depending on the tests to have the correct behavior) is to cross-reference the routing logic in the other mutation commands and make sure it's consistent.