-
Notifications
You must be signed in to change notification settings - Fork 21
CNDB-15608 fix lint issues in affected files #2131
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
Conversation
Checklist before you submit for review
|
4d30c51 to
baf1b76
Compare
|
CNDB's PR: https://github.com/riptano/cndb/pull/16068 |
The reported issue is not related to this PR change.
Failures are not related to this PR change and also happens in another PR, e.g., #2132. |
|
Changed to draft while investigating CI failures in https://github.com/riptano/cndb/pull/16068 |
ff41118 to
b8819d1
Compare
Unrelated and known test failures |
| private void forEach(Consumer<Index.Indexer> action) | ||
| { | ||
| indexers.forEach(action::accept); | ||
| indexers.forEach(action); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this might even save an unnecessary object allocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove perSSTableComponents too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might lead to memory leaks. I will re-test it, since previously I had more than one change, so the memory leak might related to similar but different change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove perSSTableComponents too?
I tested removing it from unnecessary storing as a class field and several tests failed with memory leaks. This looks to me a tech debt and potential source for memory leak bugs in future, and fixing it would require refactoring.
@michaeljmarshall what do you think about it? Is it important to create an issue and fix in future?
While working on CNDB-15608, IntelliJ lint complains were noticed, which are not related to the actual changes in the patch port. Thus I fix them in this separate commit to avoid unnecessary noise while working on the actual patch port. The changes include: - Remove unused imports - Remove unnecessary boilerplate code for unnamed Comparator construction - Use the formatter of the logger instead of string concatenation - Use method instead of lambda - Remove unnecessary suppression of resource warnings - Simplify Boolean conditions - Remove unnecessary modifiers in interfaces - Fix typos - Fixing links in javadoc comments - Add static modifier to nested classes - Remove class fields when not used - Remove unnecessary throws in method signatures - Use final when recommended - Remove unused method arguments - Replace single char strings with chars - Remove unnecessary null variable initialization - Replace assert true with assert equal - Change order of assert arguments to have expected value first - Remove unnecessary explicit casting
997a073 to
6b6de5f
Compare
This reverts commit 6b6de5f.
|
❌ Build ds-cassandra-pr-gate/PR-2131 rejected by Butler6 regressions found Found 6 new test failures
Found 3 known test failures |
Some of the issues were detected memory leaks and the change was reverted. See #2131 (comment) Other failures are not related to the PR change. |



While working on CNDB-15608, IntelliJ lint complains were noticed, which are not related to the actual changes in the patch port. Thus I fix them in this separate commit to avoid unnecessary noise while working on the actual patch port.
Many of the changes are align what I have already merged earlier in other PRs.
Some of the changes might not match preferences from others and I am open for discussion.
The changes include: