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

Testing scrubMode != "" compares references, not values #180

Open
bhalevy opened this issue Nov 2, 2021 · 2 comments
Open

Testing scrubMode != "" compares references, not values #180

bhalevy opened this issue Nov 2, 2021 · 2 comments
Milestone

Comments

@bhalevy
Copy link
Member

bhalevy commented Nov 2, 2021

Change 70b19e6 introduce a scrubMode parameter to scrub.
But,

misuses Java by comparing scrubMode to an empty string using the != operator.
As @haaawk noted, this is wrong as in Java != compares references, not values.

@avikivity
Copy link
Member

Please in the future file issues in a way that indicates user impact.

avikivity added a commit to scylladb/scylladb that referenced this issue Nov 2, 2021
* tools/jmx 5c383b6...48d37f3 (1):
  > StorageService: scrub: fix scrubMode is empty condition

Ref scylladb/scylla-jmx#180.
@bhalevy
Copy link
Member Author

bhalevy commented Nov 3, 2021

Please in the future file issues in a way that indicates user impact.

Agreed. I'm not exactly sure what the user impact is in this case. I think that the user might pass an empty scrub mode and we won't detect it here.

But storage_service scrub api handles an empty scrub_mode as the legacy api and it looked at the skip_corrupted option in this case and in any case it defaults to the ABORT scrub_mode which is exactly what will happen if the test condition detected the empty string correctly.

So bottom line, modulo testing I think there should be no user impact.

@DoronArazii DoronArazii added this to the Backlog milestone May 15, 2023
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 a pull request may close this issue.

3 participants