Conversation
Add a new flag called `--store.strict-mode` as agreed per https://thanos.io/proposals/202001_thanos_query_health_handling.md/ Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Test out if the storeRefs are properly maintained with strict mode being on/off. Test if nodes are properly separated into dynamically specified ones and statically specified. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
GiedriusS
force-pushed
the
querier_strict_mode
branch
from
b349578 to
be9e58e
Compare
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Nice, looks awesome, some thoughts though.
pkg/query/storeset_test.go
Outdated
|
|
||
| // New stores should be loaded. | ||
| storeSet.Update(context.Background()) | ||
| storeSet.Update(context.Background(), false) |
There was a problem hiding this comment.
Generally a good rule is to avoid Boolean variables in function. Especially in this case it might be better to just have different function... WDYT?
There was a problem hiding this comment.
I agree that an enum such as QuerierEvictionPolicy with values like StrictMode and LaxMode (or NormalMode) would make things cleaner here. However, you've mentioned that you'd like to see another function for this. Could you elaborate on that?
Reflects the reality better that we return both static and dynamic nodes. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Add more information on what the new flag means. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
povilasv
left a comment
povilasv
left a comment
There was a problem hiding this comment.
Really nice work 👍 I don't really have any issues with the PR
There was a problem hiding this comment.
Thanks all good as implemented as in design ❤️
However, I just realized that there might be a better UX even if we would allow instead of strict-mode flag adding a new --store-strict next to --store so you can fine granularly add certain stores as strict. I definitely like this more... It looks like with your implementation it's the matter of few tweaks to make this happen.
WDYT @GiedriusS ?
Sorry for this, but I literally did not think about this idea when we were reviewing proposal (:
It's ok but I hope that you understand where the frustration might come from. Without the other discussions that we've had, I can only see marginal UX improvement where the HELP message would be close to the place where the StoreAPIs are defined. I think it's debatable whether it will make sense to have some statically specified StoreAPIs with Cortex's All in all, I will remake this PR with your suggestion and update the proposal. I really want everyone to be happy with this and other things that we do hence why I have made that proposal in the first place. |
3 participants
Changes
Adds
--store.strict-modeflag to Thanos Query as agreed on https://thanos.io/proposals/202001_thanos_query_health_handling.md/. The changes seem straightforward:StoreSpechas a new methodStatic()which tells us if a node has been specified statically. This seems to be the right abstraction layer.Finally, marks the proposal as "done".
Verification
Unit tests pass which cover the new functionality.