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

fix(recordings): empty archive directories are now removed #957

Merged
merged 5 commits into from
May 27, 2022

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented May 26, 2022

Fixes #951

There is a bug that is tracked with the spotbugs bug tracker

[ERROR] Medium: Nullcheck of entries at line 242 of value previously dereferenced in io.cryostat.recordings.RecordingArchiveHelper.pathIsEmpty(Path) [io.cryostat.recordings.RecordingArchiveHelper, io.cryostat.recordings.RecordingArchiveHelper] At RecordingArchiveHelper.java:[line 242]Redundant null check at RecordingArchiveHelper.java:[line 243] RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE

I'm not sure why because I don't see a clear null checking in the code I added.

@maxcao13 maxcao13 added the fix label May 26, 2022
@andrewazores
Copy link
Member

There is a bug that is tracked with the spotbugs bug tracker

[ERROR] Medium: Nullcheck of entries at line 242 of value previously dereferenced in io.cryostat.recordings.RecordingArchiveHelper.pathIsEmpty(Path) [io.cryostat.recordings.RecordingArchiveHelper, io.cryostat.recordings.RecordingArchiveHelper] At RecordingArchiveHelper.java:[line 242]Redundant null check at RecordingArchiveHelper.java:[line 243] RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE

I'm not sure why because I don't see a clear null checking in the code I added.

Ah, yea. I've seen this before. IIRC SpotBugs is doing its analysis on the compiled bytecode, not directly on the Java source code you wrote. The try-with-resources construct gets compiled down to bytecode that performs a null check and so SpotBugs sometimes flags that as redundant. I thought we had got around that by updating our SpotBugs version at some point, but maybe not.

spotbugs/spotbugs#756

You can just add a @SuppressFBWarnings annotation to ignore the false positive. Example:

@maxcao13
Copy link
Member Author

running mvn clean verify on my laptop builds and passes all tests fine, i've run git status and git log and nothing seems wrong there...

@andrewazores
Copy link
Member

Looks like just a flakey test run. Those reports-related integration tests are usually the most problematic ones.

@maxcao13 maxcao13 marked this pull request as ready for review May 27, 2022 02:28
@andrewazores andrewazores merged commit 06f1a58 into cryostatio:main May 27, 2022
@maxcao13 maxcao13 deleted the bug-fix-branch branch May 27, 2022 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Empty archive directories are not removed
2 participants