[improve][misc] Use shared Jackson ObjectMapper to reduce overhead and remove ThreadLocal solution #19160
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lhotari
force-pushed
the
lh-use-thread-local-ObjectMapper
branch
from
c8ef4d8 to
05ef22c
Compare
lhotari
force-pushed
the
lh-use-thread-local-ObjectMapper
branch
from
05ef22c to
10772b8
Compare
lhotari
requested review from
cbornet,
hangc0276,
michaeljmarshall and
nicoloboschi
4 tasks
Technoboy-
changed the title
nicoloboschi
approved these changes
Jan 9, 2023
Codecov Report
@@ Coverage Diff @@
## master #19160 +/- ##
============================================
- Coverage 47.89% 47.36% -0.53%
+ Complexity 10871 10729 -142
============================================
Files 713 713
Lines 69730 69723 -7
Branches 7496 7497 +1
============================================
- Hits 33394 33024 -370
- Misses 32631 32974 +343
- Partials 3705 3725 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
|
HQebupt
approved these changes
Jan 9, 2023
cbornet
reviewed
Jan 9, 2023
pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/GenericJsonWriter.java
Show resolved
Hide resolved
- ObjectMapper is thread safe
lhotari
changed the title
Member
Author
|
@nicoloboschi PTAL . I have added a ProtectedObjectMapper class which prevents mutating the shared ObjectMapper config. |
nicoloboschi
approved these changes
Jan 12, 2023
Contributor
nicoloboschi
left a comment
nicoloboschi
left a comment
There was a problem hiding this comment.
LGTM
I like the new pattern 👍
cbornet
reviewed
Jan 12, 2023
| Optional<Long> version) { | ||
| try { | ||
| byte[] content = ObjectMapperFactory.getThreadLocal().writeValueAsBytes(policies); | ||
| byte[] content = ObjectMapperFactory.getMapper().writer().writeValueAsBytes(policies); |
Contributor
There was a problem hiding this comment.
How about having ObjectMapperFactory.getWriter() and ObjectMapperFactory.getReader() methods ?
Member
Author
There was a problem hiding this comment.
I was thinking of that, but the challenge is that there's also the Yaml versions and the ObjectMapper configured to filter null fields. That would result in a lot of similar methods. It's a tradeoff.
Leaving ObjectMapperFactory as-is and creating a completely new class would be another option. I'd suggest that it's left as another PR to continue refactoring.
…sses - prevents classloader & class leaks
…s to empty caches - this is useful in tests to prevent class / classloader leakages
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Creating a Jackson ObjectMapper is an expensive operation. A thread local ObjectMapper instance is already used in most locations.
Additional motivation is to be able to use a properly configured ObjectMapper instance with Jackson Java 8 modules. That PR is #19161 .
Modifications
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: lhotari#124