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(recordingoptions): client may explicitly unset options #640

Merged
merged 4 commits into from
Aug 11, 2021

Conversation

andrewazores
Copy link
Member

Related to #632
Fixes #634

Trimmed stacktrace output makes uncaught unit test exceptions difficult
to debug at a glance
@andrewazores andrewazores force-pushed the recording-options-unset branch from 25146c0 to bfeeb45 Compare August 10, 2021 21:35
@andrewazores andrewazores force-pushed the recording-options-unset branch from e9e0ba4 to e56bcb1 Compare August 10, 2021 21:39
@hareetd
Copy link
Contributor

hareetd commented Aug 10, 2021

Out of curiosity, how exactly was not being able to explicitly unset the recording options leading to the test failures from before?

@andrewazores
Copy link
Member Author

Out of curiosity, how exactly was not being able to explicitly unset the recording options leading to the test failures from before?

This seems to have been the key: https://github.com/cryostatio/cryostat/pull/640/files#diff-5191e54db18231ddfc4a99eb2dd6289336bc68561507563c9c471d4f8986b0d7L74

When retrieving the options from the JVM, the default value being unset is reported as 0. But, explicitly setting a value and then "resetting" it to 0 seems to turn out to actually tell the target JVM to not record anything at all, as in the recording may have a maximum size of 0 bytes.

There's probably a better/correct way to get the GET recordingOptions data from the JMC API we're using to retrieve this so that we can return data to the client in the first place that distinguishes unset options from options set to 0/false, too.

hareetd
hareetd previously approved these changes Aug 10, 2021
Copy link
Contributor

@hareetd hareetd left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

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

Besides this last question, looks good

@andrewazores andrewazores merged commit a703e48 into cryostatio:main Aug 11, 2021
@andrewazores andrewazores deleted the recording-options-unset branch August 11, 2021 14:47
tthvo pushed a commit to tthvo/cryostat-legacy that referenced this pull request Dec 9, 2024
…6.2 to 4.8.6.3 (cryostatio#640)

build(deps): bump com.github.spotbugs:spotbugs-maven-plugin

Bumps [com.github.spotbugs:spotbugs-maven-plugin](https://github.com/spotbugs/spotbugs-maven-plugin) from 4.8.6.2 to 4.8.6.3.
- [Release notes](https://github.com/spotbugs/spotbugs-maven-plugin/releases)
- [Commits](spotbugs/spotbugs-maven-plugin@spotbugs-maven-plugin-4.8.6.2...spotbugs-maven-plugin-4.8.6.3)

---
updated-dependencies:
- dependency-name: com.github.spotbugs:spotbugs-maven-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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 this pull request may close these issues.

itest failures due to ordering, failed/lack of cleanup
3 participants