Skip to content
This repository has been archived by the owner on Jan 28, 2022. It is now read-only.

various updates to capture & materialization protocols #5

Merged
merged 4 commits into from
Sep 7, 2021

Conversation

jgraettinger
Copy link
Member

Related to factoring out materialization connector images.

Re-define driver checkpoints to be patches which are applied
to a fully reduced driver checkpoint managed by the Flow runtime.

Issue #173
RecvMsg is more efficient, especially as connectors are factored
into images.

Issue #173
Allow users of the `flow` package to directly access Gazette types
rather than also having to import those Gazette protocol packages.
@jgraettinger jgraettinger marked this pull request as ready for review September 2, 2021 20:52
@jgraettinger jgraettinger requested review from a team and snowzach and removed request for a team September 2, 2021 20:52
Copy link
Contributor

@saterus saterus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems pretty reasonable. I was honestly expecting there to be more changes, but I think that was my misunderstanding about how far along we were with the Materialization protocol. Reviewing this helped put a lot of specifics to the concepts I've heard us discuss. 👍

Comment on lines -183 to +187
json_name = "driverCheckpoint"
json_name = "driverCheckpointMergePatch"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is that a full checkpoint will "patch" into any previous state with by overwriting everything, which while redundant, should still be correct, right?

Is there ever a circumstance that we will need to differentiate actual patches from full state checkpoints?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is that a full checkpoint will "patch" into any previous state with by overwriting everything, which while redundant, should still be correct, right?

That's right.

Is there ever a circumstance that we will need to differentiate actual patches from full state checkpoints?

  • It means a checkpoint can no longer represent object properties which are intentionally null.
  • It also means that intentional deletion of fields by omitting them in a future checkpoint no longer works.

That said, I'm not aware of any current connectors where these are a problem, and the Airbyte folks say they're on board. So likely to change in the future but 🤷

Comment on lines +57 to +58
// - The driver server sends TransactionResponse.Opened after ensuring
// that other raced clients are fenced (if fencing is supported).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably a bit tangential, but what exactly do you mean by "fencing" in this context? I'm not familiar with this terminology.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terminology comes from memory barriers. A fence is an operation which inserts a barrier such that a (distributed) actor F is assured no other actor Z (often called a "zombie") is able to race a modification of a mutually shared resource. F has "fenced" the resource from Z.

Comment on lines +189 to +191
if count != 1 {
return pb.NewValidationError("expected one of Open, Load, Prepare, Store, Commit")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a common go idiom for treating a struct like an enum?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I've seen enough of it to call it idiomatic. Mostly I don't see it checked, and it makes me ☹️ .

Generics are finally coming to Go, but I'd dearly love me some Rust-like algebraic enums and pattern matching....

@jgraettinger jgraettinger merged commit aadf3b6 into main Sep 7, 2021
@jgraettinger jgraettinger deleted the johnny/materialization-connectors branch September 7, 2021 22:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants