Implement the dual mode for Thanos Receiver #4231

yashrsharma44 · 2021-05-13T08:44:06Z

Implementation PR for the Thanos Split Receiver proposal - https://thanos.io/tip/proposals/202012_receive_split.md/

PR is incomplete, pushed for tracking changes and quick review
TODO:

Define behaviour when one mode is enabled, and other mode's behaviour is accessed
Tests and Docs

Signed-off-by: Yash Sharma [email protected]

cmd/thanos/receive.go

yashrsharma44 · 2021-05-18T06:54:16Z

Hi @squat !

I have added an e2e test scenario about creating 3 ingestor(hashrings contain only the local endpoint) and 1 distributor. Can you check if the logic for the same is implemented correctly?

I ran the tests, and cross-checked if the behaviour is correct, but would be nice to have your opinion as well 💯

yashrsharma44 · 2021-05-18T06:56:20Z

cmd/thanos/receive.go

-			port := parts[len(parts)-1]
-			*localEndpoint = net.JoinHostPort(hostname, port)
-		}
+		// if *localEndpoint == "" {


Do you want me to uncomment this part? However, we might not be able to support the distributor mode, as the localEndpoint would always have a non-nil value.

We should remove this if it's not used.

Yup, this is a breaking change, so lets make a note for this

kakkoyun

This is looking however it gives me an unsettling feeling. It's really hard to follow up on changes and modes.

Could we try to encapsulate logic into functions to increase readability?

I know this was an existing issue but this is a chance to improve the control flow.

yashrsharma44 · 2021-05-20T11:32:23Z

I know this was an existing issue but this is a chance to improve the control flow.

Can you link the issue?

yashrsharma44 · 2021-05-20T12:08:28Z

I know this was an existing issue but this is a chance to improve the control flow.

Can you link the issue?

Tracking issue - #2248

kakkoyun

This is on a good path. Thanks a lot @yashrsharma44.

I have added comments inline.

kakkoyun · 2021-05-20T14:05:20Z

cmd/thanos/receive.go

+	grpcProbe *prober.GRPCProbe,
+
+) error {
+	g.Add(func() error {


This looks it does the job. I'd prefer keeping the run.Group in the top-level function and extract whatever we pass to g.Add on a different function.

test/e2e/receive_test.go

cmd/thanos/receive.go

Signed-off-by: Yash Sharma <[email protected]> remove flag for ingestion mode, and decide internally Signed-off-by: Yash Sharma <[email protected]> moved grpc setup for both ingestion and distribution Signed-off-by: Yash Sharma <[email protected]> added an e2e test Signed-off-by: Yash Sharma <[email protected]> move dbs and writer outside scope Signed-off-by: Yash Sharma <[email protected]> test remove stray at /handler 467 Signed-off-by: Yash Sharma <[email protected]> fixed a duplicate code in receiver.go Signed-off-by: Yash Sharma <[email protected]> removed stray line in handler Signed-off-by: Yash Sharma <[email protected]> added a e2e test for ingestion and distributor scenario Signed-off-by: Yash Sharma <[email protected]> uncommented other tests Signed-off-by: Yash Sharma <[email protected]> restored receiver to main branch implementation Signed-off-by: Yash Sharma <[email protected]> break down multiple logical parts of receiver into functions Signed-off-by: Yash Sharma <[email protected]>

Signed-off-by: Yash Sharma <[email protected]>

cmd/thanos/receive.go

Signed-off-by: Yash Sharma <[email protected]>

yashrsharma44 · 2021-05-24T18:04:48Z

I might set up this branch and play with the benchmarks using this PR - thanos-io/thanosbench#34

So will report the statistics and benchmarks, once I run them up 😎

cmd/thanos/receive.go

test/e2e/e2ethanos/services.go

squat

Overall looks great to me! I think the PR is a lot simpler now without the extra concepts of the different "modes" :))

Co-authored-by: Lucas Servén Marín <[email protected]> Update cmd/thanos/receive.go Co-authored-by: Lucas Servén Marín <[email protected]> Signed-off-by: Yash Sharma <[email protected]>

Signed-off-by: Yash Sharma <[email protected]>

kakkoyun

🥇 🚀

bwplotka reviewed May 13, 2021

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

cmd/thanos/receive.go Outdated Show resolved Hide resolved

squat reviewed May 14, 2021

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

squat reviewed May 14, 2021

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

yashrsharma44 force-pushed the receiver-spliter branch 8 times, most recently from 7f5c0a2 to f21f32a Compare May 18, 2021 06:45

yashrsharma44 marked this pull request as ready for review May 18, 2021 06:54

yashrsharma44 commented May 18, 2021

View reviewed changes

kakkoyun reviewed May 19, 2021

View reviewed changes

This was referenced May 19, 2021

receiver: Split - Upstream the changes from the PR #3580 #3845

Closed

[WIP] receiver: Split #3580

Closed

yashrsharma44 force-pushed the receiver-spliter branch 4 times, most recently from f75150b to 15cc6f8 Compare May 20, 2021 13:45

kakkoyun reviewed May 20, 2021

View reviewed changes

yashrsharma44 force-pushed the receiver-spliter branch from 15cc6f8 to 29870d8 Compare May 20, 2021 17:02

yashrsharma44 added 2 commits May 20, 2021 23:02

added an e2e test and refactored some more code for receiver

bd7f920

Signed-off-by: Yash Sharma <[email protected]>

corrected logic for receiver

51d6791

Signed-off-by: Yash Sharma <[email protected]>

yashrsharma44 force-pushed the receiver-spliter branch 2 times, most recently from b363cc0 to 939748e Compare May 20, 2021 18:33

remove default endpoint

a12006c

Signed-off-by: Yash Sharma <[email protected]>

yashrsharma44 force-pushed the receiver-spliter branch from 939748e to a12006c Compare May 21, 2021 06:25

move run grpc to setup grpc

4be0b7b

Signed-off-by: Yash Sharma <[email protected]>

sonatype-lift bot reviewed May 21, 2021

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

merged setup and run grpc server groups

b635613

Signed-off-by: Yash Sharma <[email protected]>

yashrsharma44 force-pushed the receiver-spliter branch from d55a0aa to b635613 Compare May 21, 2021 06:55

vovanloc0798 previously approved these changes May 21, 2021

View reviewed changes

squat reviewed May 28, 2021

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

squat reviewed May 28, 2021

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

squat reviewed May 28, 2021

View reviewed changes

test/e2e/e2ethanos/services.go Outdated Show resolved Hide resolved

squat previously approved these changes May 28, 2021

View reviewed changes

yashrsharma44 dismissed stale reviews from squat and vovanloc0798 via 5473f5b May 28, 2021 12:08

Update cmd/thanos/receive.go

3dfc12d

Co-authored-by: Lucas Servén Marín <[email protected]> Update cmd/thanos/receive.go Co-authored-by: Lucas Servén Marín <[email protected]> Signed-off-by: Yash Sharma <[email protected]>

yashrsharma44 force-pushed the receiver-spliter branch 3 times, most recently from 61b7497 to ab9efaa Compare May 28, 2021 16:50

changed the e2e test name

36866ab

Signed-off-by: Yash Sharma <[email protected]>

yashrsharma44 force-pushed the receiver-spliter branch from ab9efaa to 36866ab Compare May 28, 2021 17:01

kakkoyun approved these changes May 31, 2021

View reviewed changes

kakkoyun enabled auto-merge (squash) May 31, 2021 12:44

kakkoyun merged commit 464f8fb into thanos-io:main May 31, 2021

yashrsharma44 mentioned this pull request Jun 7, 2021

receiver: Added Docs for Receiver Split behaviour #4316

Closed

2 tasks

bill3tt mentioned this pull request Jun 18, 2021

receive: multiple failure cases in non-trivial receiver topologies #4359

Closed

bill3tt mentioned this pull request Jul 6, 2021

Only propagate write request replica for RouterIngestor receiver mode #4368

Merged

2 tasks

whitleykeith mentioned this pull request Sep 8, 2021

[bitnami/thanos] Thanos receiver dualmode bitnami/charts#7148

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the dual mode for Thanos Receiver #4231

Implement the dual mode for Thanos Receiver #4231

yashrsharma44 commented May 13, 2021 •

edited

Loading

yashrsharma44 commented May 18, 2021

yashrsharma44 May 18, 2021

kakkoyun May 19, 2021

yashrsharma44 May 20, 2021

kakkoyun left a comment

yashrsharma44 commented May 20, 2021

yashrsharma44 commented May 20, 2021

kakkoyun left a comment

kakkoyun May 20, 2021

yashrsharma44 commented May 24, 2021

squat left a comment

kakkoyun left a comment

Implement the dual mode for Thanos Receiver #4231

Implement the dual mode for Thanos Receiver #4231

Conversation

yashrsharma44 commented May 13, 2021 • edited Loading

yashrsharma44 commented May 18, 2021

yashrsharma44 May 18, 2021

Choose a reason for hiding this comment

kakkoyun May 19, 2021

Choose a reason for hiding this comment

yashrsharma44 May 20, 2021

Choose a reason for hiding this comment

kakkoyun left a comment

Choose a reason for hiding this comment

yashrsharma44 commented May 20, 2021

yashrsharma44 commented May 20, 2021

kakkoyun left a comment

Choose a reason for hiding this comment

kakkoyun May 20, 2021

Choose a reason for hiding this comment

yashrsharma44 commented May 24, 2021

squat left a comment

Choose a reason for hiding this comment

kakkoyun left a comment

Choose a reason for hiding this comment

yashrsharma44 commented May 13, 2021 •

edited

Loading