-
Notifications
You must be signed in to change notification settings - Fork 31
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
TargetConnectionManager allows concurrent connections #360
TargetConnectionManager allows concurrent connections #360
Conversation
d5cbe46
to
a3f89dd
Compare
aa382b2
to
d883e6a
Compare
d1220c3
to
56601dd
Compare
c14b12f
to
5f91be4
Compare
ed191f8
to
d1bf1de
Compare
I added an integration test that tries to exercise this, although it's tough to write a test that specifically targets the concurrency. At the least it should verify that the new implementation does work as a drop-in replacement, however. The test will output the elapsed time, and in my test runs on my laptop it did become slightly shorter on average. The difference is not huge (~19s down to ~16s), but the requests fired in the test are also ones that should not take much time at all to execute to begin with, so any savings margin here is significant IMO. The test commit is at the beginning of the series, so it's easy to check out that commit and test the pre-PR functionality and test timing, and then to check out the last commit in the series to compare to the PR changes. |
Deploy one without SSL or auth, one with just auth, and one with both
d1bf1de
to
a0795d8
Compare
a0795d8
to
3691b24
Compare
All consumers of TargetConnectionManager now use executeConnectedTask and use the cached connection semantics, with manual resource handling and closure removed
3691b24
to
5ae1983
Compare
I added a few more steps to the integration test to verify the results a little more concretely, and as a side effect the total elapsed time on my machine went from ~22s pre-PR to ~16s post-PR, so the gap is widening the more API requests are issued. This points to a positive performance impact from the parallelism/concurrency enabled here. |
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.
I don't fully understand every last line, but I understand most of how the changes you made work. There is a significant time improvement for the Interleaved... test, and it all seems to work!
src/main/java/com/redhat/rhjmc/containerjfr/net/TargetConnectionManager.java
Outdated
Show resolved
Hide resolved
…3.1 to 4.8.4.0 (cryostatio#360) 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.3.1 to 4.8.4.0. - [Release notes](https://github.com/spotbugs/spotbugs-maven-plugin/releases) - [Commits](spotbugs/spotbugs-maven-plugin@spotbugs-maven-plugin-4.8.3.1...spotbugs-maven-plugin-4.8.4.0) --- 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>
Based on top of #359 - requires subprocess to not use its own independent TargetConnectionManager
This modifies the
TargetConnectionManager
to use a timed cache ofJFRConnection
s, which by default expire (are closed and removed from cache) 90 seconds after their last access.The advantage is that targeted requests taking place within a short time period of each other - such as a web-client user navigating around to see event templates, then creating a recording, then viewing a report, etc. - can generally use one long-lived connection between the ContainerJFR backend and the target, rather than opening and closing the connection on each user operation. This significantly improves responsiveness by removing a lot of latency from targeted API responses. This also allows for concurrent connections to multiple targets, whereas previously only one open connection to a single target was allowed, and requests would be blocked until the previous connection was closed.
Integration tests should be added to exercise the multi-target scenario in a consistent way, rather than simply by manual smoketesting. This will be much easier to do after #376 .