Proposal: Modification of Receive Split proposal.

[bwplotka]

👋🏽 @Chans321 @brancz @GiedriusS @kakkoyun @squat @yashrsharma44 @metalmatze

Sorry for not paying attention to the past proposal, but when we revisited and tried to review #3845 and #3580 with @kakkoyun we found the potentially easier idea that might be more cleaner for users and maintainers. Feel free to give feedback!

First of all, thanks for all of your hard work on this, and experimental implementations in PR 🤗 We only want to propose different configuration really, but the idea stays exactly the same.

Some learnings for the future:

1. Let's not merge things prematurely. The previous proposal was missing some important parts:

   • Goals were stating implementation goals, not the usage goals. We should focus on usage goals to avoid focusing too much on implementation and not seeing other options.
   • Proposal was missing Alternatives.
   • Motivation was motivating the implementation. I think it should rather motivate GOALS, not implementation yet.
   • Motivation was not clear. Statements like Right now the receiver handles ingestion, routing and writing, thus leading to too many responsibilities in a single process. might need some proof.
   • There were some left overs like -Additionally, we touch possibility for adding a consistent hashing mechanism and buffering the incoming requests. What that means?
   • The related ticket was for something else - for different database reconfiguration @squat proposed. Given that there was not enough context for this proposal.
   • Lack of migration guide for such breaking change of already stable receive.

2. Implementation PR led to confusions.

• It was too large, ideally we can split work into smaller pieces. It was hard to review properly, agree on all items, not miss important piece etc.
• It was marked as almost ready, which led @yashrsharma44 to spend time on rebasing and cleaning all of it. Instead, it was missing many elements like documentation, migration docs, mixings, dashboards, alerts that should be at least well planned ahead. If we would merge any of those two in this state it could led to even bigger confusion.

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

[brancz]

If this is the only con, then there are simple solutions to solve this like adding the http endpoint.

[bwplotka]

This is left overall, sorry, will be more explicit

[bwplotka]

I clarified, could you double check?

[brancz]

I don't see sufficient reason here to change the direction of this project entirely when it's so close to the finish line, and two maintainers having signed off on the previous architecture. Also there is a docs PR out already.

[kakkoyun]

Looks great. With this new proposed implementation, it draws more clear migration path and it's backwards compatible. It would much easier to roll these out.

[kakkoyun]

❤️

[kakkoyun]

👍

[kakkoyun]

👍

[bwplotka]

I don't see sufficient reason here to change the direction of this project entirely when it's so close to the finish line, and two maintainers having signed off on the previous architecture. Also there is a docs PR out already.

I understand your @brancz feelings and it's entirely our bad we did not participate in this discussion, however. It's my fault. I would still love if you could take a look on 3 aspects:

1. change the direction of this project entirely

Is it entirely changing? From user there is no much different. Indeed it changing a lot of implementation wise - it avoids tons of work which is good?

2. when it's so close to the finish line

I would argue with this. I don't think it's close for proper usage by users. Also it's creating breaking changes which violates the stability guarantee we made with receive. Of course we are not in 1.0 but I would expect some migration plan for receive useage. I mentioned reasoning in #4036 (comment) point 2.

This proposal here allows to avoid tricky migration.

3. , and two maintainers having signed off on the previous architecture

What's the difference if 1, two or hundred of maintainers signed off if the alternative we propose here was not considered (at least in the view of previously written proposal)?

4. I explained the rationales in Proposal. The need of single-process receive and route you commented on is arguable and really low priority issue. The main issue is the user confusion of architectural design and each components purpose, boileplate of another command, inconsistency with Querier pattern. I would love to at least look and consider In my opinion easier solution.

[kakkoyun]

@brancz @bwplotka I also want to see this push through the finish line as quickly as possible. If could just tweak the proposal and modify the existing implementation, it'd be faster to merge this. We can even make this happen for v0.20.0 by pushing the cut of RC for a week (we just released a major version recently anyways). I'm happy to help with the effort.

Current implementation changes the interface of the existing commands drastically, there is no easy migration path using current implementation. Maybe we should acknowledge the proposed design and implementation drift apart.

FWIW, either we can proposal or implementation, I want to have these changes on the main branch. Initial benchmarks by @metalmatze proved that there are nice gains in here.

[yashrsharma44]

Some nitpick and few questions from my side.
Otherwise LGTM 🥇

[brancz]

I'm not necessarily arguing with the technological direction (I do agree this is a good, maybe better direction), but I find it worrisome that we let parts of the community work on something for a long time only to step in and "know it better" and turn over a lot of work someone has put into something. This is being everything else but welcoming to new contributors, and I've noticed this behavior more than once. Part of letting people grow is allowing them to make mistakes and not stepping in and having to be part of every conversation.

Clearly, there are enough maintainers to back this proposal up from a technical perspective, so I'm not going to block this proposal, but for the record, I want to state that I find this behavior damaging for the Thanos project as a whole. Not everyone can always be part of every conversation and we need to accept that, even if it means technological decisions may be made that an individual may not necessarily agree with.

[bwplotka]

Thanks, Frederic, this is important to all of us. As we discussed offline, there were lots of offline chats and even meeting between @kakkoyun, @Chans321 @yashrsharma44. The trigger for this proposal was actually done on pre-last Contributor Thanos Hours, which is also recorded and available here:

Topic: Thanos Contributor Office Hours
Date: Apr 1, 2021 11:00 AM Universal Time UTC

Meeting Recording:
https://zoom.us/rec/share/VoV2AVCrZYmvaPk27kSftXnTAtVEb-OQqkEiYVxfXNLFWfdSu_sNiAr7CL92888Z.seIHezcDVw2szx8f

Unfortunately, you as the mentor of @Chans321 were not updated on all of those communications, sorry for that!

It's true we should be welcoming, but it's also worth to mention we started this work in Community Bridge Q4 2020. Lot's of new ideas came after that I think are fair to consider.

Not everyone can always be part of every conversation and we need to accept that, even if it means technological decisions may be made that an individual may not necessarily agree with.

Yea, but I treat this proposal as an improvement over the past work, all reusing the previous work and foundations.

[bwplotka]

Updated, please PTAL again @yashrsharma44 @kakkoyun

Thanks for good review @yashrsharma44 !

[yashrsharma44]

LGTM 🔥 , just small questions about wording of the statement 😁

[yashrsharma44]

Resolved the issues after Office Hours, looks good to me!

[GiedriusS]

LGTM modulo that one grammar nit. It indeed sucks a lot that we have some initial implementations of the previous version but at the same time I feel that it won't be too much work to update them for this improved version as we are only changing how this new mode will be turned on, right?

[bwplotka]

Thank you all for feedback and reviews!