Skip to content
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

query: add strict mode flag #2252

Closed
wants to merge 7 commits into from

Conversation

GiedriusS
Copy link
Member

@GiedriusS GiedriusS commented Mar 10, 2020

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Adds --store.strict-mode flag to Thanos Query as agreed on https://thanos.io/proposals/202001_thanos_query_health_handling.md/. The changes seem straightforward:

  • Refactored some places where we refer to "healthy" stores - now they are not necessarily healthy but active stores to which we will send the queries;
  • Refactored to separate specified store addrs into two sets - dynamically specified and statically. This should make the flow more straightforward i.e. no changes were needed to the resolvers' part;
  • StoreSpec has a new method Static() 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.

@GiedriusS GiedriusS marked this pull request as ready for review March 10, 2020 17:40
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>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks awesome, some thoughts though.

cmd/thanos/query.go Outdated Show resolved Hide resolved
cmd/thanos/query.go Outdated Show resolved Hide resolved
@@ -450,7 +453,7 @@ func TestStoreSet_Update(t *testing.T) {
discoveredStoreAddr = append(discoveredStoreAddr, stores2.StoreAddresses()...)

// New stores should be loaded.
storeSet.Update(context.Background())
storeSet.Update(context.Background(), false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@GiedriusS GiedriusS Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bwplotka ping.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bwplotka ping.

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>
Copy link
Member

@povilasv povilasv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work 👍 I don't really have any issues with the PR

seal

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (:

@GiedriusS
Copy link
Member Author

Thanks all good as implemented as in design heart

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 query-frontend on top without the strict mode. With TLS (--grpc-client-tls-secure, #977) it's a totally different case and that's where it would make more sense.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants