receive: Unify error handling #2899

kakkoyun · 2020-07-15T16:11:59Z

The Receiver has a couple of similar error modes that could be originated from different resources (i.e local tsdb, forward requests for other peers in the ring), this PR attempts to unify how errors are returned from those paths and how the errors handled, especially for the nested errors, in order to provide consistent fail mode behaviour and make the code easier to follow.

One other reason was, for some cases because of the wrapped errors, it was wrongly categorising the root cause of the errors, this PR also attempts to fix it.

Signed-off-by: Kemal Akkoyun [email protected]

Change is not relevant to the end-user.

Changes

Unify error handling paths for fan-out forward requests.

Verification

make test-local

squat

@kakkoyun this PR introduces pretty invasive changes to how errors are counted and handled and it seems like more than just a unification IMO. In general, I am worried about such large changes to the core of the receive forwarding as this could easily introduce regressions. I would like to be very cautious about changing this logic as significantly as this PR unless there is a strong need for it. I would find it really helpful for the commit message to have a summary describing what problem(s) this PR is hoping to address to that we can see if maybe there is a simpler way to do it.

pkg/receive/handler.go

pkg/receive/handler_test.go

kakkoyun · 2020-07-20T08:23:36Z

@squat I have removed the nested counting, it should be good now.

bwplotka

I think I like the direction, but I would work on those helpers more. I think we could work on simplifying them more, it might be hard to follow.

pkg/receive/handler.go

pkg/receive/handler_test.go

pkg/receive/handler.go

brancz

I like this refactoring a lot! I think we can do a bit better on naming so it's easier to understand what the functions do from a caller perspective.

pkg/receive/handler.go

kakkoyun · 2020-07-30T14:48:12Z

@bwplotka @brancz I did my best to come up with a name. I'm too bad at naming things.

brancz

love it

kakkoyun · 2020-08-11T07:07:47Z

@squat @bwplotka Could you please have another look at this?

kakkoyun · 2020-09-01T12:57:30Z

@squat @bwplotka Could you please have another look at this?

@squat @bwplotka friendly ping!

brancz

lgtm but I think I would like another pair of eyes on this

kakkoyun · 2020-10-29T10:40:54Z

@bwplotka I think I had addressed your requests for this one. Could you have another look?

kakkoyun · 2020-12-09T13:55:16Z

@bwplotka friendly ping.

bwplotka

LGTM, just small nits 👍🏽

pkg/receive/handler.go

Signed-off-by: Kemal Akkoyun <[email protected]>

bwplotka

Thanks 💪🏽

Signed-off-by: Chans321 <[email protected]> fixes in e2e test for new receieve architecture - more fixes pending fixes in receive and e2e test of receive Signed-off-by: Chans321 <[email protected]> fixed connection reset by peer error in receive e2e tests. all e2e tests pass Signed-off-by: Chans321 <[email protected]> pkg/route: Fix unit tests route: Make hashring only work on labels route: Incorporate thanos-io#2899 fixed import issues and changed the query test signature Signed-off-by: Yash Sharma <[email protected]> made some corrections while rebasing receiver changes Signed-off-by: Yash Sharma <[email protected]> Added a method Error() to implement golang error interface Signed-off-by: Yash Sharma <[email protected]> linting Signed-off-by: Yash Sharma <[email protected]> use the new promauto package Signed-off-by: Yash Sharma <[email protected]> removed unused params Signed-off-by: Yash Sharma <[email protected]> rename method and add comments Signed-off-by: Yash Sharma <[email protected]> removed unused flags in receiver service in e2e tests Signed-off-by: Yash Sharma <[email protected]> replaced promauto with prometheus, as earlier was causing unnecessary panic Signed-off-by: Yash Sharma <[email protected]> changes for debug Signed-off-by: Yash Sharma <[email protected]> removed entry for receive test for e2e Signed-off-by: Yash Sharma <[email protected]> added a readiness probe Signed-off-by: Yash Sharma <[email protected]> fix E2E tests for receive router Signed-off-by: yeya24 <[email protected]>

Signed-off-by: Chans321 <[email protected]> fixes in e2e test for new receieve architecture - more fixes pending fixes in receive and e2e test of receive Signed-off-by: Chans321 <[email protected]> fixed connection reset by peer error in receive e2e tests. all e2e tests pass Signed-off-by: Chans321 <[email protected]> pkg/route: Fix unit tests route: Make hashring only work on labels route: Incorporate thanos-io#2899 fixed import issues and changed the query test signature Signed-off-by: Yash Sharma <[email protected]> made some corrections while rebasing receiver changes Signed-off-by: Yash Sharma <[email protected]> Added a method Error() to implement golang error interface Signed-off-by: Yash Sharma <[email protected]> linting Signed-off-by: Yash Sharma <[email protected]> use the new promauto package Signed-off-by: Yash Sharma <[email protected]> removed unused params Signed-off-by: Yash Sharma <[email protected]> rename method and add comments Signed-off-by: Yash Sharma <[email protected]> removed unused flags in receiver service in e2e tests Signed-off-by: Yash Sharma <[email protected]> replaced promauto with prometheus, as earlier was causing unnecessary panic Signed-off-by: Yash Sharma <[email protected]> changes for debug Signed-off-by: Yash Sharma <[email protected]> removed entry for receive test for e2e Signed-off-by: Yash Sharma <[email protected]> added a readiness probe Signed-off-by: Yash Sharma <[email protected]> fix E2E tests for receive router Signed-off-by: yeya24 <[email protected]> revert changes for cmd/main Signed-off-by: Yash Sharma <[email protected]> nitpicks Signed-off-by: Yash Sharma <[email protected]> removed redundant health check Signed-off-by: Yash Sharma <[email protected]> Add receive router registration Signed-off-by: Yash Sharma <[email protected]> not to be commited Signed-off-by: Yash Sharma <[email protected]> modified pkg/receive/handler.go Signed-off-by: Yash Sharma <[email protected]> modified pkg/receive/multitsdb.go Signed-off-by: Yash Sharma <[email protected]> modified pkg/route Signed-off-by: Yash Sharma <[email protected]> modified cmd/thanos/receive_route.go Signed-off-by: Yash Sharma <[email protected]> replaced MultiError with NonNilMultiError Signed-off-by: Yash Sharma <[email protected]> changed to promauto Signed-off-by: Yash Sharma <[email protected]> removed unused variables Signed-off-by: Yash Sharma <[email protected]> changed labelpb.Zlabel to timeseries Signed-off-by: Yash Sharma <[email protected]> whitespace removal and revert the labels Signed-off-by: Yash Sharma <[email protected]> use the main branch's implementation of MultiError Signed-off-by: Yash Sharma <[email protected]>

kakkoyun requested review from brancz, bwplotka and squat July 15, 2020 16:11

kakkoyun mentioned this pull request Jul 15, 2020

receive: Forward Request exponential back-off for unavailable nodes #2900

Merged

1 task

kakkoyun force-pushed the unify_receive_error_handling branch 2 times, most recently from f3f2cc5 to 02f2957 Compare July 15, 2020 16:49

squat reviewed Jul 15, 2020

View reviewed changes

pkg/receive/handler.go Outdated Show resolved Hide resolved

pkg/receive/handler.go Outdated Show resolved Hide resolved

pkg/receive/handler_test.go Show resolved Hide resolved

kakkoyun marked this pull request as draft July 16, 2020 07:00

kakkoyun force-pushed the unify_receive_error_handling branch 2 times, most recently from cb2cf99 to 44ebc68 Compare July 20, 2020 08:10

kakkoyun marked this pull request as ready for review July 20, 2020 08:29

bwplotka requested changes Jul 23, 2020

View reviewed changes

kakkoyun force-pushed the unify_receive_error_handling branch from f0f8259 to 4554982 Compare July 27, 2020 11:47

brancz reviewed Jul 30, 2020

View reviewed changes

pkg/receive/handler.go Outdated Show resolved Hide resolved

pkg/receive/handler.go Outdated Show resolved Hide resolved

brancz approved these changes Jul 31, 2020

View reviewed changes

kakkoyun requested a review from bwplotka July 31, 2020 09:21

kakkoyun requested a review from squat August 11, 2020 07:06

kakkoyun force-pushed the unify_receive_error_handling branch from 6d6695e to e754ad7 Compare September 1, 2020 12:05

kakkoyun requested a review from brancz September 2, 2020 11:27

brancz approved these changes Sep 2, 2020

View reviewed changes

kakkoyun force-pushed the unify_receive_error_handling branch from e754ad7 to a4beeaf Compare December 9, 2020 12:19

bwplotka approved these changes Dec 10, 2020

View reviewed changes

pkg/receive/handler.go Outdated Show resolved Hide resolved

pkg/receive/handler.go Outdated Show resolved Hide resolved

pkg/receive/handler.go Outdated Show resolved Hide resolved

kakkoyun added 2 commits December 17, 2020 11:05

Unify error handling

09f3309

Signed-off-by: Kemal Akkoyun <[email protected]>

Do not traverse nested multierrors

9d17f61

Signed-off-by: Kemal Akkoyun <[email protected]>

kakkoyun added 3 commits December 17, 2020 11:05

Trigger CI

1529a7e

Signed-off-by: Kemal Akkoyun <[email protected]>

Simplify

7329440

Signed-off-by: Kemal Akkoyun <[email protected]>

Rename methods

0b41518

Signed-off-by: Kemal Akkoyun <[email protected]>

kakkoyun force-pushed the unify_receive_error_handling branch 4 times, most recently from 8039222 to 2fcb946 Compare December 17, 2020 08:36

Address review issues

8ed993c

Signed-off-by: Kemal Akkoyun <[email protected]>

kakkoyun force-pushed the unify_receive_error_handling branch from 2fcb946 to 8ed993c Compare December 17, 2020 12:29

bwplotka reviewed Dec 18, 2020

View reviewed changes

bwplotka merged commit 4723967 into thanos-io:master Dec 18, 2020

kakkoyun deleted the unify_receive_error_handling branch December 18, 2020 10:23

brancz added a commit to Chans321/thanos that referenced this pull request Feb 3, 2021

route: Incorporate thanos-io#2899

2467a42

squat mentioned this pull request Sep 16, 2022

receive: Incorrect error status code with multiple AlreadyExists #5407

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

receive: Unify error handling #2899

receive: Unify error handling #2899

kakkoyun commented Jul 15, 2020 •

edited

Loading

squat left a comment

kakkoyun commented Jul 20, 2020

bwplotka left a comment

brancz left a comment

kakkoyun commented Jul 30, 2020

brancz left a comment

kakkoyun commented Aug 11, 2020

kakkoyun commented Sep 1, 2020

brancz left a comment

kakkoyun commented Oct 29, 2020

kakkoyun commented Dec 9, 2020

bwplotka left a comment

bwplotka left a comment

receive: Unify error handling #2899

receive: Unify error handling #2899

Conversation

kakkoyun commented Jul 15, 2020 • edited Loading

Changes

Verification

squat left a comment

Choose a reason for hiding this comment

kakkoyun commented Jul 20, 2020

bwplotka left a comment

Choose a reason for hiding this comment

brancz left a comment

Choose a reason for hiding this comment

kakkoyun commented Jul 30, 2020

brancz left a comment

Choose a reason for hiding this comment

kakkoyun commented Aug 11, 2020

kakkoyun commented Sep 1, 2020

brancz left a comment

Choose a reason for hiding this comment

kakkoyun commented Oct 29, 2020

kakkoyun commented Dec 9, 2020

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

kakkoyun commented Jul 15, 2020 •

edited

Loading