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.

Sign up for GitHub

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

Jump to bottom

fix(snapshot): Cleanly handle empty snapshots #743

Merged
merged 56 commits into from
Dec 9, 2021
Merged

fix(snapshot): Cleanly handle empty snapshots #743

merged 56 commits into from
Dec 9, 2021

Conversation

hareetd
Copy link
Contributor

@hareetd hareetd commented Nov 2, 2021 •
edited by andrewazores
Loading

@hareetd hareetd added the fix label Nov 2, 2021
@andrewazores
Copy link
Member

Nice work here so far. This is exactly how I would have done it.

@hareetd
Copy link
Contributor Author

hareetd commented Nov 2, 2021

Thanks, once tests are updated I'll open it up for a full review.

@andrewazores andrewazores mentioned this pull request Nov 22, 2021
Merged
@hareetd
Copy link
Contributor Author

hareetd commented Nov 29, 2021

This PR should be ready to go with just a couple of tweaks.

The first issue is that when an empty Snapshot is created and subsequently deleted, the user shouldn't receive any recording creation or deletion notifications. I was able to separate the recording deletion notification process from the empty Snapshot handling but when it comes to the creation notification it looks like I'll have to change the API handling for Snapshot creation in Api.service.tsx on the frontend.

The second issue is that the -web PR I submitted to display a warning for empty Snapshots (cryostatio/cryostat-web#329) is outdated now. The warning modal should instead say something along the lines of "Snapshot failed to create....".

I'll open a -web issue to take care of those. Feel free to review this PR whenever you have time as the only remaining change will be to bump the web client version once the -web issue is resolved.

@hareetd hareetd marked this pull request as ready for review November 29, 2021 23:53
@hareetd hareetd marked this pull request as draft December 1, 2021 01:53
@hareetd hareetd marked this pull request as ready for review December 7, 2021 00:28
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

🎉 Great news! Looks like all the dependencies have been resolved:

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

@andrewazores
Copy link
Member

Please add a commit updating the web-client submodule as well now that the relevant frontend feature PR has been merged.

Hareet Dhillon added 26 commits December 8, 2021 18:13
Remove unnecessary IOException throw declaration
e2628ca
Extract out duplicate snapshot creation/verification code into the Re…
feb00f7 
…cordingTargetHelper
Start updating tests to reflect previous change
1e20a64
Continue updating tests
73c3108
Continue updating tests
f5e03a5
Start updating SnapshotIT
e7a5069
Run spotless
c0eb1b2
Ensure the abstract request handlers handle the case of a ExecutionEx…
cd115f3 
…ception-wrapped ConnectionException due to an invalid target
Get rid of unnecessary Exception throwing
7025b38
Update RecordingTargetHelperTest
d89b5fb
Ensure target connection remains open during snapshot verification
fbcdd27
Add comment linking to TargetConnectionManager#markConnectionInUse
4f0f016
replace PushbackInputStream with InputStream
063e83d
Move response status message to body in the v1 TargetSnapshotPostHandler
a786455
Ensure Snapshot stream is closed after verification
6662d54
Cover case where Snapshot read() returns -1 during verification and a…
f1a38ea 
…dd a javadoc comment for proper usage of snapshotIsReadable(...)
Run spotless
5493ba7
Refactor RecordingTargetHelper#createSnapshot to use a ConnectionDesc…
ce3c42d 
…riptor instead of a JFRConnection
Start updating unit tests to reflect previous change
ceea89b
Bump web-client submodule to include corresponding frontend change
b6cc51d
Continue updating unit tests
9f2c56f
Finish updating unit tests
24348aa
Place successful response handling inside else-block to avoid a 'java…
a3eaeb9 
….lang.IllegalStateException: Response head already sent'
Run spotless
ccc9795
Clean up tests
dd6cc6e
Run spotless
11cc58b
@hareetd hareetd requested a review from andrewazores December 9, 2021 18:28
andrewazores
andrewazores approved these changes Dec 9, 2021
View reviewed changes
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Great work here, thanks

@andrewazores andrewazores merged commit 059e6de into cryostatio:main Dec 9, 2021
@hareetd hareetd deleted the cleanly-handle-empty-snapshots branch December 9, 2021 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewazores andrewazores andrewazores approved these changes

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cleanly handle Snapshots taken when there is no source data
2 participants
@hareetd @andrewazores