query: Add Store timeout to Thanos Query #920

povilasv · 2019-03-13T14:27:30Z

Changes

Taking over #895 as I'm affected by it too #919

@d-ulyanov has done a great job and only one thing left from that PR to fix, which I'm fixing here:

#895 (comment)

I move time out from querier to proxy

…lure when one of stores timed out

Co-Authored-By: d-ulyanov <[email protected]>

bwplotka

Minor nits really, generally I like this! Thanks (:

bwplotka · 2019-03-15T12:02:55Z

CHANGELOG.md

@@ -10,7 +10,7 @@ NOTE: As semantic versioning states all 0.y.z releases can contain breaking chan
 We use *breaking* word for marking changes that are not backward compatible (relates only to v0.y.z releases.)

 ## Unreleased
-
+- [#920](https://github.com/improbable-eng/thanos/pull/920) Add Thanos store.read-timeout. Maximum time to read response from store, which defaults to 2 minutes. If request to one of stores is timed out and store.read-timeout < query.timeout partial response will be returned. If store.read-timeout >= query.timeout one of stores is timed out the client will get no data and timeout error.


Move it to Added section and remove add word

partial response will be returned (if partial response is enabled)

bwplotka · 2019-03-15T12:05:19Z

cmd/thanos/query.go

+		queryMaxDuration := time.Duration(*queryTimeout)
+		storeReadMaxDuration := time.Duration(*storeReadTimeout)
+
+		if storeReadMaxDuration > queryMaxDuration {


Let's remove that. Context logic already ensures that

bwplotka · 2019-03-15T12:08:08Z

docs/components/query.md

@@ -270,5 +270,12 @@ Flags:
                                 if no max_source_resolution param is specified.
      --query.partial-response   Enable partial response for queries if no
                                 partial_response param is specified.
+      --store.read-timeout=2m    Maximum time to read response from store. If


same here (mention about disabling partial response)

bwplotka · 2019-03-15T12:14:03Z

pkg/query/querier_test.go

+	storeClient := &store.ClientMock{
+		StoreClientMock: &storepb.StoreClientMock{
+			SeriesCallback: func(ctx context.Context, in *storepb.SeriesRequest, opts ...grpc.CallOption) (storepb.Store_SeriesClient, error) {
+				// Just wait for ctx.Done() as it works internally in GRPC StoreClient


missing trailing period

also I would remove this, I think it's obvious.

bwplotka · 2019-03-15T12:18:46Z

pkg/query/querier_test.go

@@ -274,6 +277,54 @@ func TestDedupSeriesIterator(t *testing.T) {
 	}
 }

+func TestQuerier_StoreReadTimeout(t *testing.T) {
+	storeClient := &store.ClientMock{


I would be more comfortable if this test would have also another client that actually serves some metrics after 100 miliseconds.

bwplotka · 2019-03-15T12:19:03Z

pkg/query/querier_test.go

+	}
+
+	storeReadTimeout := 1 * time.Millisecond
+	queryTimeout := 1 * time.Second


let's make it longer otherwise slow CI will make this test flaky

bwplotka · 2019-03-15T12:26:29Z

pkg/query/querier_test.go

+
+	select {
+	case <-storeQueryDone:
+		// Do nothing - storeQuery expected to finish first


missing trailing periods at the end

bwplotka · 2019-03-15T12:27:25Z

pkg/store/mock.go

+import "github.com/improbable-eng/thanos/pkg/store/storepb"
+
+// ClientMock is a structure for mocking storepb.Client interface
+type ClientMock struct {


I think you can move it to mock_test.go file instead

bwplotka · 2019-03-15T12:29:28Z

pkg/store/proxy_test.go

@@ -51,7 +51,7 @@ func TestProxyStore_Info(t *testing.T) {
 	q := NewProxyStore(nil,
 		func() []Client { return nil },
 		component.Query,
-		nil,
+		nil, 100*time.Second,


2 * time.Minute?

bwplotka · 2019-03-15T12:30:40Z

pkg/store/storepb/mock.go

+)
+
+// StoreClientMock is a structure for mocking StoreClient interface
+type StoreClientMock struct {


I don't think this is necessary. See my comment here: #895 (comment)

d-ulyanov · 2019-03-15T13:37:03Z

@povilasv this diff seems strange, I see mock.go here (I've removed it) and some changes that I've removed too.

Lets re-create PR pls, don't you mind?

povilasv · 2019-03-15T14:58:52Z

@d-ulyanov your PR is all good now, was outdated when I started doing mine. So let's just use yours.

d-ulyanov · 2019-03-15T15:08:09Z

Okay, cool, thanks @povilasv
Let me know if you need any help :)

povilasv force-pushed the store-timeout branch 3 times, most recently from 0892fc5 to 4f385bd Compare March 15, 2019 10:21

povilasv changed the title ~~WIP: Store timeout~~ tore timeout Mar 15, 2019

Dmitry Ulyanov and others added 4 commits March 15, 2019 13:12

query: add store.read-timeout parameter to avoid partial response fai…

69fd873

…lure when one of stores timed out

query: fix errcheck in querier tests

eec6998

queery: update query.md docs

c058148

Update cmd/thanos/query.go

4aefb8a

Co-Authored-By: d-ulyanov <[email protected]>

povilasv force-pushed the store-timeout branch from 4f385bd to 3fdca8b Compare March 15, 2019 11:14

povilasv changed the title ~~tore timeout~~ Add Store timeout to Thanos Query Mar 15, 2019

Move Store timeout to Proxy

b92cf1b

povilasv force-pushed the store-timeout branch from 3fdca8b to b92cf1b Compare March 15, 2019 11:17

Add Changelog entry

6f90b05

bwplotka mentioned this pull request Mar 15, 2019

query: add store.read-timeout parameter to avoid partial response failure when one of stores timed out #895

Closed

bwplotka requested review from bwplotka and GiedriusS and removed request for bwplotka March 15, 2019 12:02

povilasv changed the title ~~Add Store timeout to Thanos Query~~ query: Add Store timeout to Thanos Query Mar 15, 2019

bwplotka requested changes Mar 15, 2019

View reviewed changes

bwplotka mentioned this pull request Mar 15, 2019

query: Add store.response-timeout #928

Merged

povilasv closed this Mar 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

query: Add Store timeout to Thanos Query #920

query: Add Store timeout to Thanos Query #920

povilasv commented Mar 13, 2019 •

edited

Loading

bwplotka left a comment

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

d-ulyanov Mar 15, 2019

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

bwplotka Mar 15, 2019

d-ulyanov commented Mar 15, 2019

povilasv commented Mar 15, 2019

d-ulyanov commented Mar 15, 2019

query: Add Store timeout to Thanos Query #920

query: Add Store timeout to Thanos Query #920

Conversation

povilasv commented Mar 13, 2019 • edited Loading

Changes

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-ulyanov commented Mar 15, 2019

povilasv commented Mar 15, 2019

d-ulyanov commented Mar 15, 2019

povilasv commented Mar 13, 2019 •

edited

Loading