Avoid introducing bookkeeper-common into the pulsar-common #9551
Conversation
--- *Motivation* Direct using jackson to parse json to avoid introduce the bookkeeper-common into the pulsar-common. That will make other modules which are using pulsar-common has some unused bookkeeper dependencies.
zymap
changed the title
| @@ -30,6 +31,8 @@ | |||
| private final Class policyClass; | |||
| private final Map<String, Object> properties; | |||
|
|
|||
| private static ObjectMapper mapper = new ObjectMapper(); | |||
There was a problem hiding this comment.
This is not thread safe and also is not used the preferred configuration for Jackson. pulsar-common has already ObjectMapperFactory.getThreadLocal() which is used for this.
There was a problem hiding this comment.
Thanks for the reminder. I change to use the ObjectMapperFactory.getThreadLocal(). Please take a look again.
There was a problem hiding this comment.
@merlimat I am not sure I get your point.
AFAIK it is very common to have static fields holding ObjectMapper if you do not need to change the configuration.
Usually you create one ObjectMapper per class or per group of classes (internally it holds a cache)
https://stackoverflow.com/questions/3907929/should-i-declare-jacksons-objectmapper-as-a-static-field
There was a problem hiding this comment.
btw using consistently ObjectMapperFactory.getThreadLocal() is good to me
There was a problem hiding this comment.
We've seen concurrency issues in the past resulting in random exceptions. If I'm not remembering wrong, they were related to the caching of the classes introspection in the object mapper.
There was a problem hiding this comment.
@merlimat that makes sense to me.
In fact usually you create a static ObjectMapper when you are sure that it will deal only with a limited and fixed set classes .
in this case the ObjectMapper will deal only with this class, and thus it is safe to use a static instance.,
As just said before, I prefer to have a consistent way of coding, so here in Pulsar is it fine to me to use ObjectManagerFactory. The overhead is to create a ObjectMapper (and the internal cache) per thread instead of having just one per target class.
|
|
||
| EnsemblePlacementPolicyConfig originalConfig = | ||
| new EnsemblePlacementPolicyConfig(MockedEnsemblePlacementPolicy.class, Collections.EMPTY_MAP); | ||
| byte[] encodedConfig = originalConfig.encode(); |
There was a problem hiding this comment.
in order to confirm backward compatibility in the future I feel it may have a good value to try to also deserialise a fixed string constant, I mean a value that has not been produced but current version of the code
what about adding such a test ?
- String encoded = "{.....}"
- decode
- assertEquals
|
/pulsarbot run-failure-tests |
* Avoid introduce bookkeeper-common into the pulsar-common --- *Motivation* Direct using jackson to parse json to avoid introduce the bookkeeper-common into the pulsar-common. That will make other modules which are using pulsar-common has some unused bookkeeper dependencies. * Fix the build and add some tests * Address comments (cherry picked from commit 18e61b3)
Motivation
Direct using jackson to parse json to avoid introduce the bookkeeper-common
into the pulsar-common. That will make other modules which are using pulsar-common
has some unused bookkeeper dependencies.