storage,compute: rethink reconciliation by benesch · Pull Request #13526 · MaterializeInc/materialize

benesch · 2022-07-08T21:04:07Z

Ignore the first commit. That's #13525.

Previously, reconciliation sat on top of storage/compute servers, and
attempted to paper over commands and responses so that clients believed
they were talking to a fresh server, while servers always believed they
were talking to the originally-connected client. This introduced a lot
of complicated logic and double bookkeeping.

This commit pushes reconciliation logic into the storaged and computed
servers. The servers are now aware of when a new client connects, and
they simply ditch any state that was related to the old client.

This tees us up for the last reconciliation task: garbage collecting
sources and dataflows that newly-connected clients are not interested
in.

Touches https://github.com/MaterializeInc/database-issues/issues/3125.
Touches https://github.com/MaterializeInc/database-issues/issues/3691.

Motivation

This PR refactors existing code.

Testing

This PR has adequate test coverage / QA involvement has been duly considered.

Release notes

This PR includes the following user-facing behavior changes:

n/a

antiguru

I think this seems good, but I still have to convince myself that it does what it advertises. If @frankmcsherry gets to review first, then don't block on me!

benesch · 2022-07-11T17:57:41Z

I think this seems good, but I still have to convince myself that it does what it advertises. If @frankmcsherry gets to review first, then don't block on me!

Yeah current plan is to have @frankmcsherry take a swing at adding in the implementation of InitializationComplete before we ship this! I think that will make it much easier to reason about whether the reconciliation logic is correct, as it will all live in one place instead of scattered about the implementation of StorageState and ComputeState.

I'm not presently planning to push this work any further myself.

frankmcsherry · 2022-07-13T17:32:33Z

Commit 2ec1e0b adds a method Worker::reconcile that embodies the reconciliation logic. It adds and subtracts a few things here and there, also. The Coordinator::bootstrap calls a new method Controller::initialization_complete which kicks off the signal (@benesch: storage puts one automatically at some places, which was wrong for compute at least).

My confidence in the correctness of this logic is .. minimal. It seems to correctly pick up existing arrangements and drop dataflows for a TAIL command, but .. as you can see it's all a bit fiddly. My guess is that we should have someone with ownership over this / related stuff, if it ever changes again in the future.

Known issues: multi-process replicas can get out of sync by receiving different prefixes of a command stream, and thereby lead to different conclusions about reconciliation. This is bad news, and we are separately discussing how to fix (in https://github.com/MaterializeInc/database-issues/issues/3881).

benesch · 2022-07-14T12:50:52Z

Seems at least as correct as the existing logic, and I've got higher confidence that we'll be able to improve this logic in the new to structure to the point of "definitely correct." At least, not having to do bookkeeping in both the reconciler module and the worker itself is easier for me to reason about.

How do you want to proceed with this PR, @frankmcsherry? Should we get it rebased and then talk @antiguru into reviewing it/taking long term ownership of it?

frankmcsherry · 2022-07-14T12:55:03Z

I'm going to try and see if I can figure out / fix some of the issues (I haven't looked into them yet) but then getting some eyes on whether the structure of things look good and improvable/maintainable works for me. I'd love to hand it off w/o a bunch of failing tests though. :D

If you are up for rebasing, I'll start sniffing around the failures.

benesch · 2022-07-14T12:57:30Z

Yes, happy to do the rebase if that would be helpful! I can also stay out of the way.

…

On Thu, Jul 14, 2022 at 8:55 AM Frank McSherry ***@***.***> wrote: I'm going to try and see if I can figure out / fix some of the issues (I haven't looked into them yet) but then getting some eyes on whether the structure of things look good and improvable/maintainable works for me. I'd love to hand it off w/o a bunch of failing tests though. :D If you are up for rebasing, I'll start sniffing around the failures. — Reply to this email directly, view it on GitHub <#13526 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGXSIH22NHL4UON4PBFRRLVUAE3FANCNFSM53CETF7A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

antiguru · 2022-07-14T13:41:47Z

I just had a look at @frankmcsherry's changes, and I think they look good! Definitely a plus on the maintainability side, and well explained.

benesch · 2022-07-16T20:16:22Z

Yes, happy to do the rebase if that would be helpful! I can also stay out
of the way.

Hearing nothing, I did the rebase!

frankmcsherry · 2022-07-17T19:01:53Z

@antiguru and @benesch this is green now. Still not 100% confident (especially if we are not testing turning things off and on again), but I'd be happy to begin the process of transferring it to a COMPUTE owner!

Make the approach to storage reconciliation match the approach to compute reconciliation, where an explicit InitializeCompaction command is used to trigger the reconciliation logic on the server. This allows stale ingestions to be removed.

benesch

I filled in the TODO(benesch)s for storage! We actually do have a decent test for storage reconciliation and it seems happy. This all LGTM, but I agree that we should get @antiguru's sign off too because either he or his appointee will be responsible for this code!

benesch · 2022-07-14T12:41:35Z

-            .state
-            .history
-            .iter()
-            .chain(&[ComputeCommand::InitializationComplete])


It made sense without the bootstrap signal! Just not once that exists.

antiguru

This looks good, thank y'all!

Co-authored-by: Moritz Hoffmann <antiguru@gmail.com>

This field is dead code. Its last/only use has been removed in MaterializeInc#13526. This commit removes the `is_subscribe` field and bring the compute `SinkToken` in line with the same type defined in `storage`.

This field is dead code. Its last/only use has been removed in #13526. This commit removes the `is_subscribe` field and bring the compute `SinkToken` in line with the same type defined in `storage`.

benesch requested review from antiguru and frankmcsherry July 8, 2022 21:04

benesch force-pushed the reconciliation-rethink branch from 350c123 to 99b7ce0 Compare July 8, 2022 22:35

antiguru reviewed Jul 11, 2022

View reviewed changes

Comment thread src/compute/src/server.rs

benesch force-pushed the reconciliation-rethink branch from 2ec1e0b to 4841017 Compare July 16, 2022 18:02

This was referenced Jul 17, 2022

compute: set a root tracing span for peek dataflows #13634

Closed

storage,compute: rethink reconciliation [PART 1] #13680

Merged

Reconcile commands in computed

d03d29f

benesch force-pushed the reconciliation-rethink branch from 4841017 to d03d29f Compare July 17, 2022 17:00

Reflect initialization in new clusters

2b77783

benesch commented Jul 17, 2022

View reviewed changes

antiguru approved these changes Jul 18, 2022

View reviewed changes

Comment thread src/compute/src/compute_state.rs Outdated

Comment thread src/storage/src/controller/rehydration.rs Outdated

frankmcsherry and others added 2 commits July 18, 2022 08:11

Update src/compute/src/compute_state.rs

f37b4bd

Co-authored-by: Moritz Hoffmann <antiguru@gmail.com>

Update src/storage/src/controller/rehydration.rs

bb251b4

Co-authored-by: Moritz Hoffmann <antiguru@gmail.com>

frankmcsherry enabled auto-merge (squash) July 18, 2022 12:12

frankmcsherry merged commit 2f9735c into MaterializeInc:main Jul 18, 2022

benesch deleted the reconciliation-rethink branch July 24, 2022 01:21

teskje mentioned this pull request Mar 30, 2023

compute: remove unused SinkToken::is_subscribe #18497

Merged

5 tasks

Conversation

benesch commented Jul 8, 2022

Motivation

Testing

Release notes

Uh oh!

antiguru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benesch commented Jul 11, 2022

Uh oh!

frankmcsherry commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benesch commented Jul 14, 2022

Uh oh!

frankmcsherry commented Jul 14, 2022

Uh oh!

benesch commented Jul 14, 2022 via email

Uh oh!

antiguru commented Jul 14, 2022

Uh oh!

benesch commented Jul 16, 2022

Uh oh!

frankmcsherry commented Jul 17, 2022

Uh oh!

benesch left a comment

Choose a reason for hiding this comment

Uh oh!

benesch Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

frankmcsherry Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

antiguru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

frankmcsherry commented Jul 13, 2022 •

edited

Loading