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

Custom targets #213

Merged
merged 8 commits into from
Jun 24, 2021
Merged

Custom targets #213

merged 8 commits into from
Jun 24, 2021

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores marked this pull request as ready for review May 17, 2021 20:02
@andrewazores andrewazores requested a review from Alexjsenn May 17, 2021 20:02
@Alexjsenn
Copy link
Contributor

Hey, so I was testing this out and I created a fake target similar to what you had suggested when testing out the api implementation, but when I select this new target and then go to the recordings tab, it crashes. I got the same result in both chrome and firefox. Do you get the same behaviour or is something wrong on my end?

@andrewazores
Copy link
Member Author

What happens when it "crashes" - is this a frontend error message or an actual backend Cryostat API error?

Did the target you defined point to a target that actually existed/was reachable?

@Alexjsenn
Copy link
Contributor

No, it did't point to anything that existed. I ran it in smoketest and I added: service:jmx:rmi:///jndi/rmi://cryostat:9099/jmxrmi
The browser tab just stops responding.

@andrewazores
Copy link
Member Author

Interesting, thanks for that note - I'll take a look at it. Sounds like I made some kind of React mistake.

@andrewazores
Copy link
Member Author

No, it did't point to anything that existed. I ran it in smoketest and I added: service:jmx:rmi:///jndi/rmi://cryostat:9099/jmxrmi
The browser tab just stops responding.

Actually, I see this behaviour on main, not just on this PR's branch. Do you see that too? I'll investigate the root cause further and open a separate PR.

@andrewazores
Copy link
Member Author

After a git bisect, the first bad revision appears to be 441b63dbbe695a5ff36fe08c3a196d9808078343 - that's #214 , which touched the ActiveRecordingList, so that looks plausible.

@Josh-Matsuoka if you have some free cycles, could you take another look at that? If not then I'll take it on.

@andrewazores
Copy link
Member Author

@Alexjsenn for the purposes of testing the functionality in this PR, I'm going to rebase it onto main as of a couple of commits back and force-push it here.

Try adding service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi as a custom target definition as well - this should instruct the Cryostat JVM to connect back to itself, so you can easily test out a target definition that does actually point to something.

@Alexjsenn
Copy link
Contributor

@Alexjsenn for the purposes of testing the functionality in this PR, I'm going to rebase it onto main as of a couple of commits back and force-push it here.

Try adding service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi as a custom target definition as well - this should instruct the Cryostat JVM to connect back to itself, so you can easily test out a target definition that does actually point to something.

Ok, it works perfectly now!

Alexjsenn
Alexjsenn previously approved these changes May 19, 2021
@Josh-Matsuoka
Copy link
Contributor

@andrewazores hey, I'll take a look at that bug this week, sorry about that.

@andrewazores
Copy link
Member Author

@andrewazores hey, I'll take a look at that bug this week, sorry about that.

No worries, I should have caught it in review as well.

@andrewazores andrewazores merged commit 007b073 into cryostatio:main Jun 24, 2021
@andrewazores andrewazores deleted the custom-target branch June 24, 2021 17:42
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.

3 participants