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

compute: wire up a persist client on initialization #12272

Closed

Conversation

petrosagg
Copy link
Contributor

Motivation

This is a piece of the big storage/compute separation PR that I split off. The compute instances will need to be spun up with command line flags that specify the persist location they should expect to find the collections stored by the storage controller so this patch wires this up.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Copy link
Contributor

@frankmcsherry frankmcsherry left a comment

Choose a reason for hiding this comment

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

I don't mind, but is there an explainer why this is a command line argument (hard to change) as opposed to part of the source instantiation description (easier to change)?

@petrosagg
Copy link
Contributor Author

@frankmcsherry It depends on our expectation regarding whether or not per-source persist locations is something that we're interested in providing. The storage controller was already wired up with command line parameters to a specific persist location and I also think about the persist location as a property of the "region environment" that is shared among all components in the cluster.

@petrosagg
Copy link
Contributor Author

Sorry, not the storage controller, I meant storaged was wired up already with command line parameters

@frankmcsherry
Copy link
Contributor

I'm thinking more that changes to the command line arguments of computed are a "harder" upgrade, as it requires a few more moving parts to change at the same time. In principle, a STORAGE-private struct that describes what it needs to connect to persist can be evolved privately. Or at least, that burden is something to think about!

@petrosagg
Copy link
Contributor Author

I think this is a good point you're raising. I'll change this PR to pass additional storage metadata down to storage as part of the dataflow description, and have that metadata carry the persist location information and anything else needed in the future

@petrosagg
Copy link
Contributor Author

@frankmcsherry new approach here: #12276

@petrosagg petrosagg closed this May 5, 2022
@petrosagg petrosagg deleted the compute-persist-client branch May 5, 2022 13:01
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

2 participants