Use a virtual threads friendly pool with Jackson #4920
Conversation
|
merge is on hold until 2.16.0 is released |
src/main/java/io/vertx/core/json/jackson/HybridJacksonPool.java
Outdated
Show resolved
Hide resolved
|
@mariofusco @franz1981 we will release 4.5.0 this week, I'm assuming we cannot yet merge this PR ? |
The final release of Jackson 2.16 is not out yet ( see https://central.sonatype.com/artifact/com.fasterxml.jackson.core/jackson-core/versions ) so, unless it is ok for you to depend on that 2.16.0-rc1, I'm afraid that this PR cannot be merged yet. |
|
I will move this issue to 4.5.1
…On Wed, Nov 15, 2023 at 11:07 AM Mario Fusco ***@***.***> wrote:
@mariofusco <https://github.com/mariofusco> @franz1981
<https://github.com/franz1981> we will release 4.5.0 this week, I'm
assuming we cannot yet merge this PR ?
The final release of Jackson 2.16 is not out yet ( see
https://central.sonatype.com/artifact/com.fasterxml.jackson.core/jackson-core/versions
) so, unless it is ok for you to depend on that 2.16.0-rc1, I'm afraid that
this PR cannot be merged yet.
—
Reply to this email directly, view it on GitHub
<#4920 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABXDCUB72QKRBTKEXAMYQ3YESH6TAVCNFSM6AAAAAA6RO4ZCSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJSGE2TEMBWGE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
src/main/java/io/vertx/core/json/jackson/HybridJacksonPool.java
Outdated
Show resolved
Hide resolved
|
As you know, Jackson 2.16.0 is released |
mariofusco
force-pushed
the
jackson_pool
branch
from
86830dd to
fc84b66
Compare
|
@vietj I think that this is ready to be merged now. I bumped the jackson version with this other PR vert-x3/vertx-dependencies#157 |
|
@mariofusco Please add some tests to be sure that it works as expected; by mocking the thread id(s) and verifying it hits/distribute the requests and/or reuse the same pool, if expected |
There was a problem hiding this comment.
@mariofusco Please add some tests to be sure that it works as expected (including not getting any weird array index out of bound exception); by mocking the thread id(s) and verifying it hits/distribute the requests and/or reuse the same pool, if expected.
Additionally, add something (I would use a counter into the node(s)) to report the current size of the pool;
this type of telemetry will help to troubleshoot errors.
Decide if it makes sense to add a fastthreadlocal-only version to replace the current existing one too, and give vertx users a boost without relying on the existing implementation from Jackson (which uses SoftRef too, bleah!)
src/main/java/io/vertx/core/json/jackson/HybridJacksonPool.java
Outdated
Show resolved
Hide resolved
src/main/java/io/vertx/core/json/jackson/HybridJacksonPool.java
Outdated
Show resolved
Hide resolved
|
@franz1981 I implemented what you suggested and added a couple of simple tests. Please let me know if there is something more that I could check or improve. |
|
@vietj @franz1981 is this ok to be merged? |
src/test/java/io/vertx/core/json/jackson/HybridJacksonPoolTest.java
Outdated
Show resolved
Hide resolved
|
@vietj @franz1981 I modified my test case to use virtual threads if they are available and completely skip the test otherwise. Please let me know if there is something else to be done here. If not I don't see any further reason to hold this pull request. |
src/test/java/io/vertx/core/json/jackson/HybridJacksonPoolTest.java
Outdated
Show resolved
Hide resolved
|
Hi @vietj is there any other outstanding issue why you're holding this PR? (not rushing, just checking if I missed to do something) |
@vietj I addressed one and replied to the other. |
|
@mariofusco can you squash everything in a single commit with a nice commit message :-) ? |
|
@mariofusco last thing; please can you write a draft PR in quarkus to make sure everything here is done for there? |
mariofusco
force-pushed
the
jackson_pool
branch
from
d49defa to
13d60ce
Compare
|
@mariofusco I see some activity on this PR, is it safe to merge ? |
@vietj I squashed everything as you asked, but now I'm also testing the Quarkus integration as suggested by @franz1981 ( see quarkusio/quarkus#38196 ). Please just give me a bit of time to check that it is also working as expected. |
|
@vietj @franz1981 I'm sorry, but I tested this integration and found a serious bug in I'm afraid that this pull request cannot be merged yet, till the problem won't be fixed in jackson and for sure we cannot use jackson 2.16.1 for this. |
|
thanks for raising this @mariofusco so for now it will be on hold |
|
@vietj @franz1981 @geoand I further investigated the problem, found the root cause and with my last commit made my hybrid pool compatible with jackson-databind behaviour. I also test that with this fix all Quarkus integration tests in the jackson extension are green. I'm still discussing with @cowtowncoder if what jackson-databind does is correct or not, or if we can find a way to improve it. Anyway with my fix that discussion is now unrelated with this pool implementation and now I believe that it is totally safe to merge this as it is. @vietj if it is ok for you, please feel free merging this pull request. |
|
Hi @vietj, the problem that I found is now fixed in Jackson, but as I wrote in my former comment, with my last commit I found a solution that works even without that fix and I checked that this behaves correctly also on the quarkus integration side. Is there any outstanding work here or any other reason why you're still holding this pull request? |
|
@mariofusco merged, thanks for the contribution, can you port this to master branch ? |
|
|
Awesome thanks!
…On Wed, Jan 31, 2024 at 10:36 AM Mario Fusco ***@***.***> wrote:
@mariofusco <https://github.com/mariofusco> merged, thanks for the
contribution, can you port this to master branch ?
@vietj <https://github.com/vietj> see #5084
<#5084>
—
Reply to this email directly, view it on GitHub
<#4920 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABXDCQEAGEP7LKQLWMGTYDYRIGB3AVCNFSM6AAAAAA6RO4ZCSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJYG4ZDQNZXGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This pull request is a follow up of the work done to make the internal pool used by Jackson configurable and pluggable. It is not intended to be merged until Jackson 2.16 won't be released. Also this is the porting of the same pull request sent to quarkus.