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

Java: Allow uncovered slots ClusterScan #2859

Merged
merged 4 commits into from
Dec 24, 2024

Conversation

jamesx-improving
Copy link
Collaborator

@jamesx-improving jamesx-improving commented Dec 24, 2024

Issue link

This Pull Request is linked to issue (URL): #2436

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Commits will be squashed upon merging.

@jamesx-improving
Copy link
Collaborator Author

jamesx-improving commented Dec 24, 2024

Due to the fact that we can't create/stop valkey clusters in the middle of a Java test, and because the testing of this feature requires dedicated cluster with special configuration, the automatic test is not included in this PR.

Created Issue #2860 to track the progress of implementing the ability to create/terminate cluster in the middle of a Java test. Will finish CI testing of this feature afterwards.

I've done test locally and the result is expected.

Here's a screenshot of the test result:
image

@@ -28,6 +29,13 @@ public class ScanOptions extends BaseScanOptions {
*/
private final ObjectType type;

/**
* If set to True, the scan will perform even if some slots are not covered by any node. It's
Copy link
Collaborator

@yipin-chen yipin-chen Dec 24, 2024

Choose a reason for hiding this comment

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

Suggested change
* If set to True, the scan will perform even if some slots are not covered by any node. It's
* If set to true, the scan will perform even if some slots are not covered by any node. It's

Also next line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very much a nitpick, just some grammar things:

  • "If set to true, the scan will proceed/be performed/be executed even if..."
  • "However, it is important to note that in this mode, the scan cannot guarantee coverage of all keys in the cluster and it loses the ability to validate its progress"

Up to you if you want to adopt any of these.

Copy link
Collaborator

@yipin-chen yipin-chen left a comment

Choose a reason for hiding this comment

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

LGTM, after minor comment update.
If possible, can you please provide manual testing steps?

@yipin-chen
Copy link
Collaborator

This PR will be merged into the main branch along with PR #2814 and PR #2815. Once merged, all fixes for issue #2436 will be backported to the Release-1.2 branch.

Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we need to freeze this PR until we can run tests for this feature in CI.

@Yury-Fridlyand Yury-Fridlyand added the java issues and fixes related to the java client label Dec 24, 2024
Co-authored-by: tjzhang-BQ <[email protected]>
Signed-off-by: James Xin <[email protected]>
@yipin-chen
Copy link
Collaborator

LGTM, but I think we need to freeze this PR until we can run tests for this feature in CI.

I have asked James to create an issue to track the missing capability for cluster creation in Java. As discussed in this morning's sync-up meeting, we will merge this PR with proof of manual testing.

Signed-off-by: James Xin <[email protected]>
@jamesx-improving jamesx-improving merged commit 9767ead into main Dec 24, 2024
15 of 16 checks passed
@jamesx-improving jamesx-improving deleted the java/jamesx-allow-ucslots-scan-base-main branch December 24, 2024 22:02
prateek-kumar-improving pushed a commit that referenced this pull request Dec 27, 2024
Java: Allow uncovered slots ClusterScan

---------

Signed-off-by: James Xin <[email protected]>
Co-authored-by: tjzhang-BQ <[email protected]>
niharikabhavaraju pushed a commit to niharikabhavaraju/valkey-glide that referenced this pull request Dec 29, 2024
Java: Allow uncovered slots ClusterScan

---------

Signed-off-by: James Xin <[email protected]>
Co-authored-by: tjzhang-BQ <[email protected]>
BoazBD pushed a commit to BoazBD/valkey-glide that referenced this pull request Jan 1, 2025
Java: Allow uncovered slots ClusterScan

---------

Signed-off-by: James Xin <[email protected]>
Co-authored-by: tjzhang-BQ <[email protected]>
Signed-off-by: BoazBD <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1_2_1_candidate java issues and fixes related to the java client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants