lhotari commented on PR #19160: URL: https://github.com/apache/pulsar/pull/19160#issuecomment-1378862088
> @lhotari I see the improvement here, a couple of comments: > > * wouldn't be better to only expose the read/write method + `unsafeGetInstance` ? My concern is that you could call `ObjectMapperFactory.getInstance().setDateFormat(xx)` changing the internal configuration You are right. I assume you mean returning an ObjectReader/ObjectWriter (which are immutable) instead of returning a ObjectMapper at all. It would be a better approach. One detail is that the current threadlocal solution is already "unsafe". > * `ObjectMapperFactory` naming should matches the singleton pattern There's a need to have different type of ObjectMapper instances already. I noticed that replacing `new ObjectMapper()` with the shared instance broke things since the serialization config if different (`Include.ALWAYS` is the default, but the configured one has `Include.NON_NULL`). Therefore the name `ObjectMapperFactory` could be justified although it's not an actual Factory. What name would you suggest? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
