-
Notifications
You must be signed in to change notification settings - Fork 590
HDDS-10122. GSON requires tweaks for Java 17. #6147
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
Conversation
|
@ArafatKhan2198 please check test failures |
|
Thanks @ArafatKhan2198 for continuing work on this. There are still some test failures. |
|
The forked CI passed successfully :- https://github.com/ArafatKhan2198/ozone/actions/runs/8308711361 |
|
@adoroszlai Could you please take a look |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ArafatKhan2198 for the patch.
To make the change more manageable, I suggest splitting it to separate patches:
ozone tenantozone admin namespaceozone debug- rest of the change, including
EventQueue(still has GSON), and removal ofgsonas a dependency
Created HDDS-10538 with sub-tasks for this purpose.
(Please see few inline comments, too.)
| String subPath = subPathDU.get("path").toString(); | ||
| // differentiate key from other types | ||
| if (!(boolean)subPathDU.get("isKey")) { | ||
| LinkedTreeMap<?, ?> subPathDU = (LinkedTreeMap<?, ?>) o; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LinkedTreeMap is part of GSON. I guess if we reach this code, ClassCastException will be thrown. Probably needs to be Map, but please confirm by testing it manually.
| ArrayList<LinkedTreeMap> taskStatusList = objectMapper.readValue( | ||
| taskStatusResponse, new TypeReference<ArrayList<LinkedTreeMap>>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace LinkedTreeMap with Map here, too?
| ObjectMapper mapper = new ObjectMapper(); | ||
| // For pretty-printing | ||
| mapper.enable(SerializationFeature.INDENT_OUTPUT); | ||
| // To serialize null values | ||
| mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL); | ||
| String jsonReport = mapper.writeValueAsString(containerJson); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to avoid creating a new ObjectMapper for each serialization (also in ~17 other files). Use JsonUtils:
ozone/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/server/JsonUtils.java
Lines 59 to 74 in 238bff0
| public static String toJsonStringWithDefaultPrettyPrinter(Object obj) | |
| throws IOException { | |
| return WRITER.writeValueAsString(obj); | |
| } | |
| public static String toJsonString(Object obj) throws IOException { | |
| return MAPPER.writeValueAsString(obj); | |
| } | |
| public static ArrayNode createArrayNode() { | |
| return MAPPER.createArrayNode(); | |
| } | |
| public static ObjectNode createObjectNode(Object next) { | |
| return MAPPER.valueToTree(next); | |
| } |
|
Thanks @ArafatKhan2198 for splitting this. |
What changes were proposed in this pull request?
This issue is related to the use of GSON in the codebase and its compatibility with Java 17. The proposed solution is to replace GSON with Jackson to reduce dependencies.
Affected Modules:
The replacement of GSON with Jackson is considered for several modules in the project, including:
KeyValueContainerMetadataInspectorandTestKeyValueContainerMetadataInspectorin thehadoop-hdds/container-servicemodule.HddsConfServletandEventQueuein thehadoop-hdds/frameworkmodule, along with its corresponding tests.hadoop-ozone/common, especially related to multitenancy and access policiesAccessPolicy&
RangerAccessPolicy.hadoop-ozonemodule, such asTestReconWithOzoneManager,TestOzoneShellHA, and various classes in the tools sub-module related to Ozone Shell, tenant management, and debug tools.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10122?filter=-1
How was this patch tested?
Currently Draft State will be adding more classes into it.