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

Add ReplicationConfigs to SessionConfig for excluding keyspaces from replica precompute #831

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelhly
Copy link

We add ReplicationOptions which includes a vector of keyspace names to SessionConfig. When instantiating ClusterData, we exclude keyspaces from provided by ReplicationOptions when computing PrecomputedReplicas to conserve memory/compute resources.

Fixes: #671

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@michaelhly michaelhly force-pushed the replication_opts branch 2 times, most recently from bcd496f to d20d1ea Compare October 11, 2023 17:54
@michaelhly
Copy link
Author

michaelhly commented Oct 11, 2023

@piodul @wprzytula @havaker would you mind taking a quick look? Happy to write tests and finish up docs if this looks like it's in the right track.

@michaelhly michaelhly force-pushed the replication_opts branch 2 times, most recently from 9f61a0a to 2b965fe Compare October 11, 2023 21:19
@michaelhly michaelhly changed the title Add ReplicationOptions to SessionConfig for excluding keyspaces from replica precompute Add ReplicationConfigs to SessionConfig for excluding keyspaces from replica precompute Oct 11, 2023
@michaelhly michaelhly force-pushed the replication_opts branch 3 times, most recently from 42a1742 to fa8fbd5 Compare October 11, 2023 21:37
…replica precompute

We add ReplicationOptions which includes a vector of keyspace names to SessionConfig.
When instantiating ClusterData, we exclude keyspaces from provided by ReplicationOptions when
computing PrecomputedReplicas to conserve memory/compute resources.
@piodul
Copy link
Collaborator

piodul commented Oct 13, 2023

@Lorak-mmk please review

@piodul piodul requested a review from Lorak-mmk October 13, 2023 10:38
@Lorak-mmk
Copy link
Collaborator

You can already choose which keyspaces to precompute (see keyspaces_to_fetch) in SessionConfig, but:

  • it is an allowlist, while your approach is denylist. I'm wondering if maybe the best approach is to allow both (make enum Allowlist(Vec) and Denylist(Vec) instead of just Vec).
  • It prevents info about other keyspaces to be fetched at all (so it's not available in ClusterData), while your solution only avoids precomputing replication strategies.

So I think this change may be valuable. I'll try to post initial review soon.

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.

Provide a way to configure which keyspaces need replica set precomputation
3 participants