Skip to content

Conversation

ByteSizedJoe
Copy link

Description

This PR adds a new httpclient-vertx-5 module that provides Vert.x 5.x HTTP client support for the Fabric8 Kubernetes client. This addresses issue #7174 by implementing improved async handling and WebSocket separation using the latest Vert.x 5.0.1.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@ByteSizedJoe
Copy link
Author

Hey @shawkins @manusa! I think this was the approach that was mentioned in the previous PR. Let me know what your thoughts are and I'll proceed getting the documentation and other tasks complete!

@shawkins
Copy link
Contributor

Hey @shawkins @manusa! I think this was the approach that was mentioned in the previous PR. Let me know what your thoughts are and I'll proceed getting the documentation and other tasks complete!

Thank you for this new pr @ByteSizedJoe. Yes this does look like the approach that is needed. Overall things look good as the changes you made on top of the 4 implemention seemed to make sense. If there are refinements you think would be good to have for the vertx 4 implemenation, please do open a separate pr for that.

Other than changing the dependency, are there any migration issues of note you have seen (memory footprint, threads used, etc.)?

We will probably want to add vertx5 into the https://github.com/fabric8io/kubernetes-client/blob/main/.github/workflows/e2e-httpclient-tests.yml and the chaos test workflow.

@ByteSizedJoe
Copy link
Author

Hey @shawkins @manusa! I think this was the approach that was mentioned in the previous PR. Let me know what your thoughts are and I'll proceed getting the documentation and other tasks complete!

Thank you for this new pr @ByteSizedJoe. Yes this does look like the approach that is needed. Overall things look good as the changes you made on top of the 4 implemention seemed to make sense. If there are refinements you think would be good to have for the vertx 4 implemenation, please do open a separate pr for that.

Other than changing the dependency, are there any migration issues of note you have seen (memory footprint, threads used, etc.)?

We will probably want to add vertx5 into the https://github.com/fabric8io/kubernetes-client/blob/main/.github/workflows/e2e-httpclient-tests.yml and the chaos test workflow.

Hey @shawkins. Just wanted to do a quick update. I'm working on throwing together a small A/B test harness to get a comparison of Vertx 4/5. I'll post my findings here.

I'll also work on getting the tests added.

@ByteSizedJoe
Copy link
Author

Hey @shawkins!

Okay so the app that I am interested in using this for has never been built with vertx4 so I couldn't do a direct swap and compare. So what I did instead was setup a simple A/B test harness: https://github.com/ByteSizedJoe/k8s-client-ab-testing

This test harness uses minikube, jcmd, jstat and performs some k8s operations and measures jvm metrics.

This is what I found:

Metric Vert.x 4 Vert.x 5 Delta % Change
Threads 57 37 -20.00 -35.1%
NMT Total MB 337.82 401.82 +64.00 +18.9%
NMT Heap MB 50.00 154.00 +104.00 +208.0%
YGC (count) 83 24 -59.00 -71.1%
FGC (count) 0 0 +0.00 -
GCT (s) 4 4 +0.00 +0.0%

I'll attach the raw output from jcmd/jstat here as well.

I understand this isn't a real world integration test but I hope that this helps satisfy what you're looking for. Let me know your thoughts!

out-mvn.zip

I have also added vertx-5 to the E2E matrix.

@shawkins
Copy link
Contributor

Thank you @ByteSizedJoe for your analysis and continued work on the pr. To clarify by NMT heap you mean the amount of on heap memory reported by native memory tracking? That bumb is the only thing that looks concerning - adding in @vietj for additional thoughts on vertx 4 vs 5.

@ByteSizedJoe
Copy link
Author

Thank you @ByteSizedJoe for your analysis and continued work on the pr. To clarify by NMT heap you mean the amount of on heap memory reported by native memory tracking? That bumb is the only thing that looks concerning - adding in @vietj for additional thoughts on vertx 4 vs 5.

Yep! This is from jcmd vm.native_memory summary.

For what it's worth, I re-ran for a much longer benchmark and got:

Metric Vert.x 4 Vert.x 5 Delta % Change
Threads 58 57 -1.00 -1.7%
NMT Total MB 661.27 599.31 -61.96 -9.4%
NMT Heap MB 336.00 312.00 -24.00 -7.1%
YGC (count) 13,263 14,075 +812.00 +6.1%
FGC (count) 0 0 +0.00 -
GCT (s) 4 28 +24.00 +600.0%

So interestingly NMT heap looks better but more GCT time was up 😄

@ByteSizedJoe
Copy link
Author

Hey @shawkins and team! Just wanted to check in here if there's anything else I can do to help with this at the moment. Let me know! Thank you.

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.

2 participants