Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Improvement] refactor JsonUtils.objectMapper() #3313

Closed
FANNG1 opened this issue May 9, 2024 · 3 comments · Fixed by #3334
Closed

[Improvement] refactor JsonUtils.objectMapper() #3313

FANNG1 opened this issue May 9, 2024 · 3 comments · Fixed by #3334
Assignees
@FANNG1
Labels
0.5.1 Release v0.5.1 0.6.0 Release v0.6.0 improvement Improvements on everything

Comments

@FANNG1
Copy link
Contributor

FANNG1 commented May 9, 2024 •
edited

What would you like to be improved?

There are possible problems about current implement:

  1. class method conflict when the application depends both on client-java-runtime and common, this's uncommon, but exists like spark connector rely on client-java-runtime and run integrate test in embed mode relay on common module.
// for `common` module
  public static com.fasterxml.jackson.databind.ObjectMapper objectMapper() {
    return ObjectMapperHolder.INSTANCE;
  }

// in `client-java-runtime` module
  public static  com.datastrato.gravitino.shaded.com.fasterxml.jackson.databind.ObjectMapper objectMapper() {
    return ObjectMapperHolder.INSTANCE;
  }
  1. use a globle static ObjectMapper which could be changed unexpectedly, it's hard to maintance.
GravitinoClientBase.java
    ObjectMapper mapper = JsonUtils.objectMapper();
    mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);

current implement:

  private static class ObjectMapperHolder {
    private static final ObjectMapper INSTANCE =
        JsonMapper.builder()
            .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false)
            .configure(EnumFeature.WRITE_ENUMS_TO_LOWERCASE, true)
            .enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS)
            .build()
            .registerModule(new JavaTimeModule());
  }

  public static ObjectMapper objectMapper() {
    return ObjectMapperHolder.INSTANCE;
  }

How should we improve?

  1. move objectMapper factory from common to client and server seperately.
  2. create a new objectMapper when used.
@FANNG1 FANNG1 added the improvement Improvements on everything label May 9, 2024
@diqiu50
Copy link
Contributor

diqiu50 commented May 9, 2024 •
edited

The client-java-runtime includes the common module. Why do you need both modules?

The ObjectMapper is mostly common, except for certain configurations. I believe making it a singleton is not a good idea. Creating a new object each time can help avoid mutual interference. In practice, the ObjectMapper in the common module is used only with the client or server, and they are not in the same process, except during our tests.

@yuqi1129
Copy link
Contributor

yuqi1129 commented May 9, 2024

The client-java-runtime includes the common module. Why do you need both modules?

+1. The module spark-connetor should only depend on one of them, so maybe you need to figure out why this happens. In my opinion, if there is no clear need, using a singleton is reasonable.

@FANNG1
Copy link
Contributor Author

FANNG1 commented May 9, 2024 •
edited

@yuqi1129 @diqiu50 , spark-connector depends on client-java-runtime and miniGravitino server depends on common, this happens when run sparkIT in embedded mode. And why current integration-test module not encounter this problem, because integration-test module depends on client module and so it loads JsonUtils from client module, I guess.

@FANNG1 FANNG1 mentioned this issue May 11, 2024
Merged
@FANNG1 FANNG1 self-assigned this May 15, 2024
jerryshao pushed a commit that referenced this issue May 16, 2024
@FANNG1
[#3313] feat(common): refactor JsonUtils object mapper to client and …
2d27893 
…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
github-actions bot pushed a commit that referenced this issue May 16, 2024
@FANNG1 @web-flow
[#3313] feat(common): refactor JsonUtils object mapper to client and …
758290c 
…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
@jerryshao jerryshao mentioned this issue May 16, 2024
Merged
@jerryshao jerryshao added 0.5.1 Release v0.5.1 0.6.0 Release v0.6.0 labels May 16, 2024
yuqi1129 pushed a commit that referenced this issue May 16, 2024
@jerryshao @FANNG1
[#3313] feat(common): refactor JsonUtils object mapper to client and …
e895b02 
…service side (#3411)

### 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

Co-authored-by: FANNG <xiaojing@datastrato.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@FANNG1 FANNG1

Labels
0.5.1 Release v0.5.1 0.6.0 Release v0.6.0 improvement Improvements on everything
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[#3313] feat(common): refactor JsonUtils object mapper to client and service side FANNG1/gravitino
4 participants
@jerryshao @FANNG1 @diqiu50 @yuqi1129