storage: convert Arc source tokens back to Rcs#12825
Merged
Merged
Conversation
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.
guswynn
approved these changes
Jun 1, 2022
guswynn
left a comment
guswynn
left a comment
Contributor
There was a problem hiding this comment.
As long as the pg-cdc test pass, we know this still works! but of course it will, rustc compiling this means it has the same behavior!
we never moved them back because it worked fine, but I don't suspect it will ever need to be arc's again. However, leaving it with the upgrade/activate-on-drop style makes it way easier to move back in the future, so thanks!
| // failures are ignored | ||
| let _ = self.activator.activate(); | ||
| } | ||
| _activator: Rc<ActivateOnDrop<()>>, |
Contributor
There was a problem hiding this comment.
oh yeah, i forgot there is a non-sync version of this already in timely
Contributor
Author
|
Ok great, tyvm for the review @guswynn! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@guswynn @petrosagg I pulled this out of #12798 since that didn't get landed. I addressed your feedback from that review, Gus! I don't have all the context paged in on why we would/wouldn't want to do this, but given that these are thread local right now I found the
Arcs confusing. But perhaps y'all know of some reason why they'll need to become thread safe again!PR #12082 converted source tokens to thread-safe
Arcs to be compatiblewith the TCP storage/compute boundary, but since #12216 replaced the TCP
boundary with persist we can go back to Rcs.
Motivation
Testing
Release notes
This PR includes the following user-facing behavior changes: