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.

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

[#3313] feat(common): refactor JsonUtils object mapper to client and service side #3334

Merged
merged 2 commits into from
May 16, 2024

Conversation

FANNG1
Copy link
Contributor

@FANNG1 FANNG1 commented May 11, 2024

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

@FANNG1 FANNG1 marked this pull request as draft May 11, 2024 01:11
@FANNG1 FANNG1 force-pushed the object_mapper branch 3 times, most recently from c4429c7 to 1711636 Compare May 11, 2024 08:59
@FANNG1 FANNG1 marked this pull request as ready for review May 11, 2024 14:20
@FANNG1
Copy link
Contributor Author

FANNG1 commented May 13, 2024

@yuqi1129 @diqiu50 , could you help to review this PR when you have time, thanks

*/
public static ObjectMapper objectMapper() {
@VisibleForTesting
public static ObjectMapper objectMapperForTest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's only for testing, @VisibleForTesting seems to be enough, the name objectMapperForTest is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@FANNG1
Copy link
Contributor Author

FANNG1 commented May 13, 2024

@yuqi1129 could you help to review again?

@FANNG1
Copy link
Contributor Author

FANNG1 commented May 13, 2024

@jerryshao do you have time to review ?

@FANNG1
Copy link
Contributor Author

FANNG1 commented May 15, 2024

@yuqi1129 do you have any more comments?

@yuqi1129
Copy link
Contributor

@yuqi1129 do you have any more comments?

No, I'm okay with the current changes.

@FANNG1
Copy link
Contributor Author

FANNG1 commented May 15, 2024

@jerryshao do you have time to review this?

@FANNG1 FANNG1 self-assigned this May 15, 2024
@jerryshao
Copy link
Contributor

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a private constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/**
* Retrieves a globally shared {@link ObjectMapper} instance.
*
* <p>Note: This ObjectMapper is shared across all users. If you need to modify the default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shared across all users, I think it is not accurate for server-side object mapper, you need to improve the comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* an {@link ObjectMapper} from the appropriate {@code ObjectMapperProvider} on the server or
* client side.
*
* @return the shared {@link ObjectMapper} instance for tests.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your comments should be differentiated between server and client. Currently, it is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@jerryshao
Copy link
Contributor

We don't have to backport this to branch-0.5, seems like this is just a refractory. WDYT? @FANNG1

@yuqi1129
Copy link
Contributor

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.

@jerryshao
Copy link
Contributor

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.

@FANNG1
Copy link
Contributor Author

FANNG1 commented May 16, 2024

@jerryshao @yuqi1129 , this PR blocks running spark multi-version integrate test in embedded mode #3305 . so it needs to backport 0.5.1

@jerryshao jerryshao merged commit 2d27893 into apache:main May 16, 2024
22 checks passed
github-actions bot pushed a commit that referenced this pull request May 16, 2024
…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
diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Jun 13, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] refactor JsonUtils.objectMapper()
3 participants