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

HDDS-10542. Replace remaining GSON usage with Jackson. #6500

Merged
merged 11 commits into from
May 6, 2024

Conversation

ArafatKhan2198
Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 commented Apr 9, 2024

What changes were proposed in this pull request?

This pull request introduces a migration from Gson to Jackson, specifically focusing on replacing Gson's JsonParser with Jackson's ObjectMapper. It involves using ObjectMapper's readValue() method to deserialize JSON strings into Java objects. Additionally, the code utilizes Jackson's JsonNode to navigate and extract data from JSON structures.

Important mention ➖

We also improved the JsonUtils class by introducing new utility methods for more flexible JSON processing with Jackson. We added readArrayFromReader to deserialize JSON content from a Reader into Java arrays, and convertValueOrObjectNode to efficiently convert any given value into a JsonNode, ensuring compatibility with various JSON structures. These improvements streamline JSON operations across our project, for both serialization and deserialization tasks.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10542

How was this patch tested?

Ran the Unit Tests for the changed classes.
The forked CI also ran green!

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ArafatKhan2198 for working on this.

@adoroszlai
Copy link
Contributor

@ArafatKhan2198 Let's remove changes related to EventQueue from this patch, and address that in a follow-up task.

@ArafatKhan2198
Copy link
Contributor Author

ArafatKhan2198 commented May 1, 2024

@ArafatKhan2198 Let's remove changes related to EventQueue from this patch, and address that in a follow-up task.

Ok will remove.

But I did try a bunch of things, from the error The InvalidDefinitionException suggests that Jackson attempted to serialize an instance of UnknownFieldSet$Parser, which likely does not have any properties Jackson can serialize directly. Reading a little about it might happen because of protobuf classes.

One quick fix i tried to implement was that ➖

  • Disable FAIL_ON_EMPTY_BEANS: As a quick fix, I can configure our ObjectMapper to not fail on empty beans. This tells Jackson to output an empty JSON object ({}) instead of throwing an exception when it encounters a type with no properties to serialize.
objectMapper.disable(SerializationFeature.FAIL_ON_EMPTY_BEANS);
  • We could also use @JsonIgnoreProperties to filter and ignore certain properties or classes when serializing. But not sure what we could ignore there.Earlier in this class gson was using a custom serializer to ignore serialising for the parent field in the NodeImpl class. Jackson does not inherently support exclusion strategies like Gson's ExclusionStrategy, which dynamically includes or excludes fields from serialization at runtime. Instead, to achieve similar functionality, we have used the @JsonIgnore in the NodeImpl class annotation directly on fields we wish to exclude. Could it have been not implemented properly and this field might throw error?

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@ArafatKhan2198 , thanks for working on this!

  • We should add the test methods to src/test.
  • This change is very big (64kb). Please split it into at least two JIRAs. We may have the hadoop-hdds first and then then hadoop-ozone changes.

Copy link
Contributor

@devmadhuu devmadhuu left a comment

Choose a reason for hiding this comment

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

@ArafatKhan2198 thanks for working on this patch, few comments. Pls check.

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@ArafatKhan2198 , thanks for the update! Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13068634/6500_review.patch

BTW, please don't use wrap short lines (lines < 80 chars). Our max line length is 120 chars.

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@ArafatKhan2198 , thanks for the update. The change looks good.

  • Please address @devmadhuu 's comment on isMissing(). I also have a question there.
  • Please don't use short lines for the new code.

@ArafatKhan2198
Copy link
Contributor Author

@szetszwo @devmadhuu We should be good to go on this, let me know if there are any other changes.

Copy link
Contributor

@devmadhuu devmadhuu left a comment

Choose a reason for hiding this comment

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

Thanks @ArafatKhan2198 for working on this patch. LGTM +1

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

+1 the change looks good.

@szetszwo szetszwo merged commit f61f56d into apache:master May 6, 2024
39 checks passed
@szetszwo
Copy link
Contributor

szetszwo commented May 6, 2024

@ArafatKhan2198 , thanks for your contribution!

@devmadhuu , thanks a lot for reviewing this!

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants