[#3313] feat(common): refactor JsonUtils object mapper to client and service side #3334
Conversation
FANNG1
force-pushed
the
object_mapper
branch
3 times, most recently
from
c4429c7
to
1711636
Compare
| */ | ||
| public static ObjectMapper objectMapper() { | ||
| @VisibleForTesting | ||
| public static ObjectMapper objectMapperForTest() { |
There was a problem hiding this comment.
If it's only for testing, @VisibleForTesting seems to be enough, the name objectMapperForTest is redundant.
|
@yuqi1129 could you help to review again? |
|
@jerryshao do you have time to review ? |
|
@yuqi1129 do you have any more comments? |
No, I'm okay with the current changes. |
|
@jerryshao do you have time to review this? |
|
Sorry @FANNG1 for the delay, I will review this tomorrow. |
| * Provides a singleton {@link ObjectMapper} configured for specific serialization and | ||
| * deserialization behaviors. | ||
| */ | ||
| public class ObjectMapperProvider { |
There was a problem hiding this comment.
Can you please add a private constructor?
| /** | ||
| * Retrieves a globally shared {@link ObjectMapper} instance. | ||
| * | ||
| * <p>Note: This ObjectMapper is shared across all users. If you need to modify the default |
There was a problem hiding this comment.
shared across all users, I think it is not accurate for server-side object mapper, you need to improve the comment here.
| * an {@link ObjectMapper} from the appropriate {@code ObjectMapperProvider} on the server or | ||
| * client side. | ||
| * | ||
| * @return the shared {@link ObjectMapper} instance for tests. |
There was a problem hiding this comment.
I think your comments should be differentiated between server and client. Currently, it is the same.
|
We don't have to backport this to branch-0.5, seems like this is just a refractory. WDYT? @FANNG1 |
As you mentioned, there is no need to cherry-pick such PRs as it's only a minor refactor or improvement, what concerns me is that it may cause potential code conflicts in the future. |
|
I'm more worried about the potential unknown issues introduced by the refactoring. As 0.5.1 is a stable release, maybe we should not include such refactoring PR avoid the issues. |
|
@jerryshao @yuqi1129 , this PR blocks running spark multi-version integrate test in embedded mode #3305 . so it needs to backport 0.5.1 |
…service side (#3334) ### What changes were proposed in this pull request? 1. provide separate method to get object mapper in client and service side 2. rename `JsonUtils.objectMapper` to `JsonUtils.objectMapperForTest` ### Why are the changes needed? Fix: #3313 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests
…t and service side (apache#3334) ### What changes were proposed in this pull request? 1. provide separate method to get object mapper in client and service side 2. rename `JsonUtils.objectMapper` to `JsonUtils.objectMapperForTest` ### Why are the changes needed? Fix: apache#3313 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests
What changes were proposed in this pull request?
JsonUtils.objectMappertoJsonUtils.objectMapperForTestWhy are the changes needed?
Fix: #3313
Does this PR introduce any user-facing change?
no
How was this patch tested?
existing tests