Navigation Menu

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.

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

storage: make ReclockFollower shareable #14558

Merged
merged 1 commit into from Sep 8, 2022

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Aug 31, 2022

As part of BIR, the source reader operator will need access to the reclock trace to "invert" a resumption_frontier into a upstream commit frontier. This pr makes the ReclockFollower share-able, within a single thread (worker), by using Rc<RefCell>. This should have a relatively low cost. This pr does NOT implement the inversion logic.

Notes:

  • I chose the simplest route, which is to protect ALL data within the RefCell
  • The reclock nested-iterator had to changed to a single for_each. @petrosagg came up with a way to avoid this with Yoke, but that seems needlessly complex for our usecae.
  • Note that borrow_mut() users keep their &mut signatures. This can ensure that we reduce the chance of writing code that will hit a borrow error, while keep the api clean (ReclockFollower: Clone)

Motivation

  • This PR adds a known-desirable feature.
    part of BIR

Checklist

@guswynn guswynn changed the title temp storage: make ReclockFollower shareable Sep 1, 2022
@guswynn guswynn marked this pull request as ready for review September 1, 2022 19:59
Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

I think the code changes look good. I don't much like the pattern of things that are Clone but are only doing a shallow copy and are then sharing state. I don't know how common this is in the rust world so I'll defer to Gus and @petrosagg. We might have a shallow_clone() method or sth like it but, again, I don't know how idiomatic that would be.

src/storage/Cargo.toml Show resolved Hide resolved
src/storage/src/source/reclock.rs Outdated Show resolved Hide resolved
src/storage/src/source/reclock.rs Show resolved Hide resolved
@guswynn
Copy link
Contributor Author

guswynn commented Sep 8, 2022

@aljoscha I kindof agree, i can make a fn share(&self) -> Self instead of implementing Clone!

@guswynn guswynn enabled auto-merge (rebase) September 8, 2022 17:45
@guswynn guswynn merged commit 8c01384 into MaterializeInc:main Sep 8, 2022
@guswynn guswynn deleted the shared-reclock-trace branch September 8, 2022 19:59
@aljoscha
Copy link
Contributor

aljoscha commented Sep 9, 2022

@aljoscha I kindof agree, i can make a fn share(&self) -> Self instead of implementing Clone!

I like that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants