Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Separate storage and compute #12216

Closed
wants to merge 6 commits into from
Closed

Separate storage and compute #12216

wants to merge 6 commits into from

Conversation

petrosagg
Copy link
Contributor

@petrosagg petrosagg commented May 3, 2022 •
edited

Motivation

This PR decouples STORAGE and COMPUTE by transiting all source and table data through persist.

On the storage side, when a CreateSources command is received the corresponding ingestion dataflow is immediately instantiated and datat starts flowing into persist. This is in contrast with previous behaviour where storaged process were waiting for RenderSources messages to instantiate ingestion dataflows and publish the data to the TCP boundary. Both the RenderSources command and the TCP boundary are removed. Additionally, storaged processes are no longer responsible for handling table data and the associated Append command is removed. Instead, the storage controller directly appends the data requested to be appended by ADAPTER into the appropriate persist shard.

On the compute side, the DataflowDescription struct has been augmented with an additional storage metadata type which carries all the information required by the storage fat client running in computed processes to reach out to persist and read the correct data. Currently this type (named CollectionMetadata) carries the shard ids for the final storage collection and also the persist location details.

Even though all data is transited through persist, the shard id used is ephemeral. In some sense this PR only goes as far as using persist as a glorified TCP protocol. There will be follow up work that will make the storage controller durably record its choices for persist shard for a particular storage collection id and also change ADAPTER to anticipate storage collection being already there (adapter/storage controller reconciliation).

Fixes #12593

Testing

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

Release notes

This PR includes the following user-facing behavior changes:

@benesch benesch mentioned this pull request May 4, 2022
Closed
petrosagg added a commit to petrosagg/materialize that referenced this pull request May 4, 2022
@petrosagg
persist: make append functions accept a lot more types
19a2f57 
The previous API required that the user came up with an iterator of
references to values but that meant that every called that had owned
values had to write unecessary boilerplate. For example if a user had a
`Vec<((K, V), T, D)` ready to go they would need to first make this an
iterator over `&((K, V), T, D)` and then use a map adaptor to convert
that to an iterator over `((&K, &V), &T, &D)`.

This patch changes the `append` APIs to accept any combination of owned
and borrowed values (a total of 32 combinations) which makes calling
these APIs much nicer.

The usecase for this change isn't visible in this PR because it is part
of the larger MaterializeInc#12216 PR, but I thought I'd break it up for easier
review.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg added a commit to petrosagg/materialize that referenced this pull request May 4, 2022
@petrosagg
persist: make append functions accept a lot more types
46c792f 
The previous API required that the user came up with an iterator of
references to values but that meant that every called that had owned
values had to write unecessary boilerplate. For example if a user had a
`Vec<((K, V), T, D)>` ready to go they would need to first make this an
iterator over `&((K, V), T, D)` and then use a map adaptor to convert
that to an iterator over `((&K, &V), &T, &D)`.

This patch changes the `append` APIs to accept any combination of owned
and borrowed values (a total of 32 combinations) which makes calling
these APIs much nicer.

The original usecase for this change isn't visible in this PR because it
is part of the larger MaterializeInc#12216 PR, but I thought I'd break it up for
easier review. You can see however how the usage becomes nicer in
`persist_open_loop_benchmark.rs` and also in
`src/persist-client/src/lib.rs`.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
@petrosagg petrosagg mentioned this pull request May 4, 2022
Merged
petrosagg added a commit to petrosagg/materialize that referenced this pull request May 4, 2022
@petrosagg
persist: make append functions accept a lot more types
184d9ac 
The previous API required that the user came up with an iterator of
references to values but that meant that every called that had owned
values had to write unecessary boilerplate. For example if a user had a
`Vec<((K, V), T, D)>` ready to go they would need to first make this an
iterator over `&((K, V), T, D)` and then use a map adaptor to convert
that to an iterator over `((&K, &V), &T, &D)`.

This patch changes the `append` APIs to accept any combination of owned
and borrowed values (a total of 32 combinations) which makes calling
these APIs much nicer.

The original usecase for this change isn't visible in this PR because it
is part of the larger MaterializeInc#12216 PR, but I thought I'd break it up for
easier review. You can see however how the usage becomes nicer in
`persist_open_loop_benchmark.rs` and also in
`src/persist-client/src/lib.rs`.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
@nmeagan11 nmeagan11 mentioned this pull request May 6, 2022
Closed
@philip-stoev
Copy link
Contributor

@petrosagg I understand this is the PR to end all PRs, to provide table persistence among other things. Please let me know when I can jump in and start testing it.

@petrosagg petrosagg force-pushed the separate-compute-storage branch 8 times, most recently from f4a581a to eb2d4fb Compare May 10, 2022 12:51
@petrosagg petrosagg force-pushed the separate-compute-storage branch 10 times, most recently from 665e3aa to 534a49d Compare May 16, 2022 13:33
@CLAassistant

This comment was marked as resolved.

Sign in to view
1 similar comment
@CLAassistant

This comment was marked as resolved.

Sign in to view
@petrosagg petrosagg enabled auto-merge May 25, 2022 17:34
@petrosagg petrosagg disabled auto-merge May 25, 2022 17:34
@petrosagg petrosagg enabled auto-merge (squash) May 25, 2022 17:35
@petrosagg
retire storage/compute boundary
fc54d0a 
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
@petrosagg
aljoscha code review
2e3d5f1
@petrosagg
attempt to reduce persist consensus contention
1706ba9 
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
@petrosagg
slow down default timestamp frequency
7f600a6 
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
@petrosagg
rebase fixups
c5bf2fe
@petrosagg
ignore no_block test
b33b650
umanwizard pushed a commit to umanwizard/materialize-1 that referenced this pull request May 25, 2022
@petrosagg @umanwizard
This PR decouples STORAGE and COMPUTE by transiting all source and ta…
8d4aaab 
…ble data through `persist`.

NOTE: The original PR is at MaterializeInc#12216

On the storage side, when a `CreateSources` command is received the corresponding ingestion dataflow is immediately instantiated and datat starts flowing into `persist`. This is in contrast with previous behaviour where `storaged` process were waiting for `RenderSources` messages to instantiate ingestion dataflows and publish the data to the TCP boundary. Both the `RenderSources` command and the TCP boundary are removed. Additionally, `storaged` processes are no longer responsible for handling table data and the associated `Append` command is removed. Instead, the storage controller directly appends the data requested to be appended by ADAPTER into the appropriate persist shard.

On the compute side, the `DataflowDescription` struct has been augmented with an additional storage metadata type which carries all the information required by the storage fat client running in `computed` processes to reach out to `persist` and read the correct data. Currently this type (named `CollectionMetadata`) carries the shard ids for the final storage collection and also the persist location details.

Even though all data is transited through `persist`, the shard id used is **ephemeral**. In some sense this PR only goes as far as using persist as a glorified TCP protocol. There will be follow up work that will make the storage controller durably record its choices for persist shard for a particular storage collection id and also change ADAPTER to anticipate storage collection being already there (adapter/storage controller reconciliation).

Fixes MaterializeInc#12593
@umanwizard umanwizard mentioned this pull request May 25, 2022
Merged
umanwizard pushed a commit that referenced this pull request May 25, 2022
@petrosagg @umanwizard
This PR decouples STORAGE and COMPUTE by transiting all source and ta…
8749522 
…ble data through `persist`.

NOTE: The original PR is at #12216

On the storage side, when a `CreateSources` command is received the corresponding ingestion dataflow is immediately instantiated and datat starts flowing into `persist`. This is in contrast with previous behaviour where `storaged` process were waiting for `RenderSources` messages to instantiate ingestion dataflows and publish the data to the TCP boundary. Both the `RenderSources` command and the TCP boundary are removed. Additionally, `storaged` processes are no longer responsible for handling table data and the associated `Append` command is removed. Instead, the storage controller directly appends the data requested to be appended by ADAPTER into the appropriate persist shard.

On the compute side, the `DataflowDescription` struct has been augmented with an additional storage metadata type which carries all the information required by the storage fat client running in `computed` processes to reach out to `persist` and read the correct data. Currently this type (named `CollectionMetadata`) carries the shard ids for the final storage collection and also the persist location details.

Even though all data is transited through `persist`, the shard id used is **ephemeral**. In some sense this PR only goes as far as using persist as a glorified TCP protocol. There will be follow up work that will make the storage controller durably record its choices for persist shard for a particular storage collection id and also change ADAPTER to anticipate storage collection being already there (adapter/storage controller reconciliation).

Fixes #12593
@petrosagg
Copy link
Contributor Author

superseded by #12715 to get over CLA assistant being stuck

@petrosagg petrosagg closed this May 25, 2022
auto-merge was automatically disabled May 25, 2022 23:07

Pull request was closed

benesch added a commit to benesch/materialize that referenced this pull request May 29, 2022
@benesch
compute: remove storage_addr configuration parameter
5921ea5 
Since MaterializeInc#12216 separated storage and compute by transiting all data via
the persist library, the compute layer no longer needs to communicate
directly with the storage layer. So remove the configuration parameters
that pertained to the old storage-compute network protocol.
@benesch benesch mentioned this pull request May 29, 2022
Merged
1 task
benesch added a commit to benesch/materialize that referenced this pull request May 29, 2022
@benesch
compute: remove storage_addr configuration parameter
043226b 
Since MaterializeInc#12216 separated storage and compute by transiting all data via
the persist library, the compute layer no longer needs to communicate
directly with the storage layer. So remove the configuration parameters
that pertained to the old storage-compute network protocol.
benesch added a commit to benesch/materialize that referenced this pull request May 29, 2022
@benesch
compute: remove storage_addr configuration parameter
382dc36 
Since MaterializeInc#12216 separated storage and compute by transiting all data via
the persist library, the compute layer no longer needs to communicate
directly with the storage layer. So remove the configuration parameters
that pertained to the old storage-compute network protocol.
benesch added a commit that referenced this pull request May 29, 2022
@benesch
compute: remove storage_addr configuration parameter
38de395 
Since #12216 separated storage and compute by transiting all data via
the persist library, the compute layer no longer needs to communicate
directly with the storage layer. So remove the configuration parameters
that pertained to the old storage-compute network protocol.
benesch added a commit to benesch/materialize that referenced this pull request May 31, 2022
@benesch
storage: don't render sources for tables
7078122 
Since MaterializeInc#12216, tables are now entirely handled by the controller. There
is no longer a need to send a `CreateSourceCommand` to the `storaged`
process for table sources.

We should eventually adjust the types here to enforce this statically,
but this quick fix will unblock MaterializeInc#12770.
@benesch benesch mentioned this pull request May 31, 2022
Merged
1 task
benesch added a commit that referenced this pull request May 31, 2022
@benesch
storage: don't render sources for tables
2a223f9 
Since #12216, tables are now entirely handled by the controller. There
is no longer a need to send a `CreateSourceCommand` to the `storaged`
process for table sources.

We should eventually adjust the types here to enforce this statically,
but this quick fix will unblock #12770.
benesch added a commit to benesch/materialize that referenced this pull request Jun 1, 2022
@benesch
storage: convert Arc source tokens back to Rcs
06752d7 
PR MaterializeInc#12082 converted source tokens to thread-safe `Arc`s to be compatible
with the TCP storage/compute boundary, but since MaterializeInc#12216 replaced the TCP
boundary with persist we can go back to Rcs.
benesch added a commit to benesch/materialize that referenced this pull request Jun 1, 2022
@benesch
storage: convert Arc source tokens back to Rcs
ef890df 
PR MaterializeInc#12082 converted source tokens to thread-safe `Arc`s to be compatible
with the TCP storage/compute boundary, but since MaterializeInc#12216 replaced the TCP
boundary with persist we can go back to Rcs.
@benesch benesch mentioned this pull request Jun 1, 2022
Merged
1 task
benesch added a commit that referenced this pull request Jun 1, 2022
@benesch
storage: convert Arc source tokens back to Rcs
e122681 
PR #12082 converted source tokens to thread-safe `Arc`s to be compatible
with the TCP storage/compute boundary, but since #12216 replaced the TCP
boundary with persist we can go back to Rcs.
jkosh44 pushed a commit to jkosh44/materialize that referenced this pull request Jun 2, 2022
@petrosagg @jkosh44
This PR decouples STORAGE and COMPUTE by transiting all source and ta…
1098e57 
…ble data through `persist`.

NOTE: The original PR is at MaterializeInc#12216

On the storage side, when a `CreateSources` command is received the corresponding ingestion dataflow is immediately instantiated and datat starts flowing into `persist`. This is in contrast with previous behaviour where `storaged` process were waiting for `RenderSources` messages to instantiate ingestion dataflows and publish the data to the TCP boundary. Both the `RenderSources` command and the TCP boundary are removed. Additionally, `storaged` processes are no longer responsible for handling table data and the associated `Append` command is removed. Instead, the storage controller directly appends the data requested to be appended by ADAPTER into the appropriate persist shard.

On the compute side, the `DataflowDescription` struct has been augmented with an additional storage metadata type which carries all the information required by the storage fat client running in `computed` processes to reach out to `persist` and read the correct data. Currently this type (named `CollectionMetadata`) carries the shard ids for the final storage collection and also the persist location details.

Even though all data is transited through `persist`, the shard id used is **ephemeral**. In some sense this PR only goes as far as using persist as a glorified TCP protocol. There will be follow up work that will make the storage controller durably record its choices for persist shard for a particular storage collection id and also change ADAPTER to anticipate storage collection being already there (adapter/storage controller reconciliation).

Fixes MaterializeInc#12593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benesch benesch benesch approved these changes

@teskje teskje teskje left review comments

@guswynn guswynn guswynn left review comments

@danhhz danhhz danhhz approved these changes

@aljoscha aljoscha aljoscha approved these changes

@philip-stoev philip-stoev philip-stoev approved these changes

@ruchirK ruchirK Awaiting requested review from ruchirK

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

storaged: Memory leak in the face of cancellations of DML statements
8 participants
@petrosagg @philip-stoev @CLAassistant @benesch @danhhz @aljoscha @teskje @guswynn