You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ArafatKhan2198 (via GitHub)" <gi...@apache.org> on 2024/04/09 02:48:09 UTC
[PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
ArafatKhan2198 opened a new pull request, #6500:
URL: https://github.com/apache/ozone/pull/6500
## 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 ➖
In the `EventQueue` class our Gson setup included custom serialization strategies (like excluding certain fields **`ExclusionStrategy`**) 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, marking them statically for omission in the serialization process.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567092030
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -408,18 +414,19 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject usedBytesError = buildErrorAndRepair("dBMetadata." +
- OzoneConsts.CONTAINER_BYTES_USED, usedBytesAggregate, usedBytesDB,
- keyRepairAction);
- errors.add(usedBytesError);
+ ObjectNode usedBytesError =
+ buildErrorAndRepair("dBMetadata." + OzoneConsts.CONTAINER_BYTES_USED,
+ usedBytesAggregate,
+ usedBytesDB, keyRepairAction);
+ ((ArrayNode) errors).add(usedBytesError);
Review Comment:
Thanks I got confused with something else on the casting, I have removed them thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1590254778
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java:
##########
@@ -144,18 +143,18 @@ public BadFormatException(String msg) {
}
private void processConfigTagRequest(HttpServletRequest request, String cmd,
- Writer out) throws IOException {
- Gson gson = new Gson();
+ Writer out) throws IOException {
Review Comment:
Sorry about that!
Reverted the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567093561
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -205,11 +192,14 @@ public <PAYLOAD, EVENT_TYPE extends Event<PAYLOAD>> void fireEvent(
for (EventHandler handler : executorAndHandlers.getValue()) {
queuedCount.incrementAndGet();
if (LOG.isTraceEnabled()) {
- LOG.trace(
- "Delivering [event={}] to executor/handler {}: <json>{}</json>",
- event.getName(),
- executorAndHandlers.getKey().getName(),
- TRACING_SERIALIZER.toJson(payload).replaceAll("\n", "\\\\n"));
+ String jsonPayload = serializeObject(payload);
+ if (jsonPayload != null) {
+ LOG.trace(
+ "Delivering [event={}] to executor/handler {}: <json>{}</json>",
+ event.getName(),
+ executorAndHandlers.getKey().getName(),
+ jsonPayload.replaceAll("\n", "\\\\n"));
+ }
Review Comment:
If serialization fails, we now log at debug level without payload
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on PR #6500:
URL: https://github.com/apache/ozone/pull/6500#issuecomment-2088078876
> @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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1589414033
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/server/JsonUtils.java:
##########
@@ -116,4 +121,26 @@ public static <T> List<T> readFromFile(File file, Class<T> itemType)
}
}
+ /**
+ * Reads JSON content from a Reader and deserializes it into an array of the
+ * specified type.
+ */
+ public static <T> T[] readArrayFromReader(Reader reader, Class<T[]> valueType)
+ throws IOException {
+ return MAPPER.readValue(reader, valueType);
+ }
+
+ /**
+ * Converts a JsonNode into a Java object of the specified type.
+ * @param node The JsonNode to convert.
+ * @param valueType The target class of the Java object.
+ * @param <T> The type of the Java object.
+ * @return A Java object of type T, populated with data from the JsonNode.
+ * @throws IOException
+ */
+ public static <T> T treeToValue(JsonNode node, Class<T> valueType)
+ throws IOException {
+ return MAPPER.treeToValue(node, valueType);
+ }
Review Comment:
All the new methods in `JsonUtils` are unused. Please remove them.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,60 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
+ ObjectNode containerJson = inspectContainer(kvData, store);
boolean correct = checkAndRepair(containerJson, kvData, store);
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ ObjectMapper objectMapper = new ObjectMapper();
+ objectMapper.enable(
+ SerializationFeature.INDENT_OUTPUT);
+ String jsonReport = null;
+ try {
+ jsonReport = objectMapper.writeValueAsString(containerJson);
+ if (log != null) {
+ if (correct) {
+ log.trace(jsonReport);
+ } else {
+ log.error(jsonReport);
+ }
}
+ } catch (Exception e) {
+ LOG.error("Error serializing container JSON", e);
Review Comment:
Catch the exception in `JsonUtils`
```java
+ public static String indentOutputWriteValueAsString(ObjectNode containerJson) {
+ try {
+ return INDENT_OUTPUT_MAPPER.writeValueAsString(containerJson);
+ } catch (JsonProcessingException e) {
+ return JsonUtil.toJsonString(e);
+ }
+ }
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,61 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
- boolean correct = checkAndRepair(containerJson, kvData, store);
-
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ ObjectNode containerJson = inspectContainer(kvData, store);
+ boolean correct = checkAndRepair(containerJson, kvData,
+ store);
+
+ ObjectMapper objectMapper = new ObjectMapper();
+ objectMapper.enable(
+ SerializationFeature.INDENT_OUTPUT);
+ String jsonReport = null;
+ try {
+ jsonReport = objectMapper.writeValueAsString(containerJson);
Review Comment:
We may add a new `INDENT_OUTPUT_MAPPER` as below:
```java
@@ -47,6 +48,7 @@ public final class JsonUtils {
// before use.
private static final ObjectMapper MAPPER;
private static final ObjectWriter WRITER;
+ private static final ObjectMapper INDENT_OUTPUT_MAPPER;
static {
MAPPER = new ObjectMapper()
@@ -54,6 +56,8 @@ public final class JsonUtils {
.registerModule(new JavaTimeModule())
.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
WRITER = MAPPER.writerWithDefaultPrettyPrinter();
+ INDENT_OUTPUT_MAPPER = new ObjectMapper()
+ .enable(SerializationFeature.INDENT_OUTPUT);
}
private JsonUtils() {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on PR #6500:
URL: https://github.com/apache/ozone/pull/6500#issuecomment-2094904059
@szetszwo @devmadhuu We should be good to go on this, let me know if there are any other changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1590254605
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -408,18 +399,18 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject usedBytesError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode usedBytesError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.CONTAINER_BYTES_USED, usedBytesAggregate, usedBytesDB,
keyRepairAction);
errors.add(usedBytesError);
}
// check and repair if db delete count mismatches delete transaction count.
- final JsonElement pendingDeleteCountDB = dBMetadata.get(
+ JsonNode pendingDeleteCountDB = dBMetadata.path(
OzoneConsts.PENDING_DELETE_BLOCK_COUNT);
final long dbDeleteCount = jsonToLong(pendingDeleteCountDB);
- final JsonElement pendingDeleteCountAggregate
- = aggregates.get(PendingDelete.COUNT);
+ final JsonNode pendingDeleteCountAggregate
+ = aggregates.path(PendingDelete.COUNT);
Review Comment:
Done!
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -463,32 +453,34 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject chunksDirError = buildErrorAndRepair("chunksDirectory.present",
- new JsonPrimitive(true), chunksDirPresent, dirRepairAction);
+ ObjectNode chunksDirError =
+ buildErrorAndRepair("chunksDirectory.present",
+ JsonNodeFactory.instance.booleanNode(true), chunksDirPresent,
+ dirRepairAction);
Review Comment:
Done!
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java:
##########
@@ -28,13 +28,12 @@
import java.util.Map;
import java.util.Properties;
-import com.google.common.base.Strings;
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1589738492
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/server/JsonUtils.java:
##########
@@ -116,4 +121,26 @@ public static <T> List<T> readFromFile(File file, Class<T> itemType)
}
}
+ /**
+ * Reads JSON content from a Reader and deserializes it into an array of the
+ * specified type.
+ */
+ public static <T> T[] readArrayFromReader(Reader reader, Class<T[]> valueType)
+ throws IOException {
+ return MAPPER.readValue(reader, valueType);
+ }
+
+ /**
+ * Converts a JsonNode into a Java object of the specified type.
+ * @param node The JsonNode to convert.
+ * @param valueType The target class of the Java object.
+ * @param <T> The type of the Java object.
+ * @return A Java object of type T, populated with data from the JsonNode.
+ * @throws IOException
+ */
+ public static <T> T treeToValue(JsonNode node, Class<T> valueType)
+ throws IOException {
+ return MAPPER.treeToValue(node, valueType);
+ }
Review Comment:
Thanks for letting me know!
I have made the changes.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,60 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
+ ObjectNode containerJson = inspectContainer(kvData, store);
boolean correct = checkAndRepair(containerJson, kvData, store);
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ ObjectMapper objectMapper = new ObjectMapper();
+ objectMapper.enable(
+ SerializationFeature.INDENT_OUTPUT);
+ String jsonReport = null;
+ try {
+ jsonReport = objectMapper.writeValueAsString(containerJson);
+ if (log != null) {
+ if (correct) {
+ log.trace(jsonReport);
+ } else {
+ log.error(jsonReport);
+ }
}
+ } catch (Exception e) {
+ LOG.error("Error serializing container JSON", e);
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1556930658
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,63 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
- boolean correct = checkAndRepair(containerJson, kvData, store);
-
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ // Assuming inspectContainer method returns an ObjectNode instead of JsonObject
+ // You will need to refactor inspectContainer to construct an ObjectNode using Jackson
Review Comment:
I guess these comments (and similar ones below) related to refactoring are temporary, and can be removed.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -408,18 +414,19 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject usedBytesError = buildErrorAndRepair("dBMetadata." +
- OzoneConsts.CONTAINER_BYTES_USED, usedBytesAggregate, usedBytesDB,
- keyRepairAction);
- errors.add(usedBytesError);
+ ObjectNode usedBytesError =
+ buildErrorAndRepair("dBMetadata." + OzoneConsts.CONTAINER_BYTES_USED,
+ usedBytesAggregate,
+ usedBytesDB, keyRepairAction);
+ ((ArrayNode) errors).add(usedBytesError);
Review Comment:
Cast seems to be unnecessary for `ArrayNode errors`.
```suggestion
errors.add(usedBytesError);
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -437,17 +444,16 @@ private boolean checkAndRepair(JsonObject parent,
return false;
};
- final JsonObject deleteCountError = buildErrorAndRepair(
- "dBMetadata." + OzoneConsts.PENDING_DELETE_BLOCK_COUNT,
- pendingDeleteCountAggregate, pendingDeleteCountDB,
+ final ObjectNode deleteCountError = buildErrorAndRepair("dBMetadata." + OzoneConsts.PENDING_DELETE_BLOCK_COUNT,
Review Comment:
nit: let's not change this
```suggestion
final ObjectNode deleteCountError = buildErrorAndRepair(
"dBMetadata." + OzoneConsts.PENDING_DELETE_BLOCK_COUNT,
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -437,17 +444,16 @@ private boolean checkAndRepair(JsonObject parent,
return false;
};
- final JsonObject deleteCountError = buildErrorAndRepair(
- "dBMetadata." + OzoneConsts.PENDING_DELETE_BLOCK_COUNT,
- pendingDeleteCountAggregate, pendingDeleteCountDB,
+ final ObjectNode deleteCountError = buildErrorAndRepair("dBMetadata." + OzoneConsts.PENDING_DELETE_BLOCK_COUNT,
+ pendingDeleteCountAggregate,
+ pendingDeleteCountDB,
deleteCountRepairAction);
- errors.add(deleteCountError);
+ ((ArrayNode) errors).add(deleteCountError);
Review Comment:
```suggestion
errors.add(deleteCountError);
```
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/heatmap/TestHeatMapInfo.java:
##########
@@ -745,35 +743,43 @@ public void setUp() throws Exception {
public void testHeatMapGeneratedInfo() throws IOException {
// Setup
// Run the test
- JsonElement jsonElement = JsonParser.parseString(auditRespStr);
- JsonObject jsonObject = jsonElement.getAsJsonObject();
- JsonElement facets = jsonObject.get("facets");
- JsonObject facetsBucketsObject =
- facets.getAsJsonObject().get("resources")
- .getAsJsonObject();
- ObjectMapper objectMapper = new ObjectMapper();
+ // Parse the JSON string to JsonNode
+ JsonNode rootNode = JsonUtils.readTree(auditRespStr);
- HeatMapProviderDataResource auditLogFacetsResources =
- objectMapper.readValue(
- facetsBucketsObject.toString(), HeatMapProviderDataResource.class);
- EntityMetaData[] entities = auditLogFacetsResources.getMetaDataList();
- List<EntityMetaData> entityMetaDataList =
- Arrays.stream(entities).collect(Collectors.toList());
- EntityReadAccessHeatMapResponse entityReadAccessHeatMapResponse =
- heatMapUtil.generateHeatMap(entityMetaDataList);
- assertThat(entityReadAccessHeatMapResponse.getChildren().size()).isGreaterThan(0);
- assertEquals(12, entityReadAccessHeatMapResponse.getChildren().size());
- assertEquals(25600, entityReadAccessHeatMapResponse.getSize());
- assertEquals(2924, entityReadAccessHeatMapResponse.getMinAccessCount());
- assertEquals(155074, entityReadAccessHeatMapResponse.getMaxAccessCount());
- assertEquals("root", entityReadAccessHeatMapResponse.getLabel());
- assertEquals(0.0, entityReadAccessHeatMapResponse.getChildren().get(0).getColor());
- assertEquals(0.442,
- entityReadAccessHeatMapResponse.getChildren().get(0).getChildren()
- .get(0).getChildren().get(1).getColor());
- assertEquals(0.058,
- entityReadAccessHeatMapResponse.getChildren().get(0).getChildren()
- .get(1).getChildren().get(3).getColor());
+ JsonNode facetsNode = rootNode.path("facets");
+ JsonNode resourcesNode = facetsNode.path("resources");
+
+ // Deserialize the resources node directly if it's not missing
+ HeatMapProviderDataResource auditLogFacetsResources = null;
+
+ if (!resourcesNode.isMissingNode()) {
+ auditLogFacetsResources = JsonUtils.treeToValue(resourcesNode,
+ HeatMapProviderDataResource.class);
+ }
+
+ if (auditLogFacetsResources != null) {
Review Comment:
Instead of wrapping in `if`, let's add an assumption and keep the original structure. Similarly, in other test methods, too.
```
import static org.assertj.core.api.Assumptions.assumeThat;
...
assumeThat(resourcesNode.isMissingNode()).isFalse();
HeatMapProviderDataResource auditLogFacetsResources = JsonUtils.treeToValue(resourcesNode,
HeatMapProviderDataResource.class);
...
```
BTW, when can `resources` be missing?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -165,7 +166,7 @@ public void process(ContainerData containerData, DatanodeStore store) {
}
public String process(ContainerData containerData, DatanodeStore store,
- Logger log) {
+ Logger log) {
Review Comment:
nit: please avoid changing whitespace like this
(Such indentation depends on the method's visibility, return type and name. If any of those change, indentation will be off and need to be changed again.)
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -463,32 +469,37 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject chunksDirError = buildErrorAndRepair("chunksDirectory.present",
- new JsonPrimitive(true), chunksDirPresent, dirRepairAction);
- errors.add(chunksDirError);
+ ObjectNode chunksDirError =
+ buildErrorAndRepair("chunksDirectory.present",
+ JsonNodeFactory.instance.booleanNode(true), chunksDirPresent,
+ dirRepairAction);
+ ((ArrayNode) errors).add(
+ chunksDirError); // Assuming 'errors' is an ArrayNode
Review Comment:
```suggestion
errors.add(chunksDirError);
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -79,17 +70,13 @@ public EventQueue(String threadNamePrefix) {
// The field parent in DatanodeDetails class has the circular reference
// which will result in Gson infinite recursive parsing. We need to exclude
// this field when generating json string for DatanodeDetails object
- static class DatanodeDetailsGsonExclusionStrategy
- implements ExclusionStrategy {
- @Override
- public boolean shouldSkipField(FieldAttributes f) {
- return f.getDeclaringClass() == NodeImpl.class
- && f.getName().equals("parent");
- }
-
- @Override
- public boolean shouldSkipClass(Class<?> aClass) {
- return false;
+ public String serializeObject(Object obj) {
+ try {
+ return JsonUtils.toJsonString(obj);
+ } catch (Exception e) {
+ // Handle the exception, maybe log it
+ LOG.error("Error serializing object: ", e);
Review Comment:
To test the change in `EventQueue`, please run `ozone insight logs -v scm.event-queue` in an unsecure cluster (use `ozone` compose environment).
Errors related to serialization show up in SCM log:
```
scm_1 | 2024-04-09 06:08:32,299 [IPC Server handler 2 on default port 9861] ERROR events.EventQueue: Error serializing object:
scm_1 | com.fasterxml.jackson.databind.exc.InvalidDefinitionException: No serializer found for class com.google.protobuf.UnknownFieldSet$Parser and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS) (through reference chain: org.apache.hadoop.hdds.scm.server.SCMDatanodeHeartbeatDispatcher$NodeReportFromDatanode["report"]->org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos$NodeReportProto["unknownFields"]->com.google.protobuf.UnknownFieldSet["parserForType"])
scm_1 | at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:77)
scm_1 | at com.fasterxml.jackson.databind.SerializerProvider.reportBadDefinition(SerializerProvider.java:1330)
scm_1 | at com.fasterxml.jackson.databind.DatabindContext.reportBadDefinition(DatabindContext.java:414)
scm_1 | at com.fasterxml.jackson.databind.ser.impl.UnknownSerializer.failForEmpty(UnknownSerializer.java:53)
scm_1 | at com.fasterxml.jackson.databind.ser.impl.UnknownSerializer.serialize(UnknownSerializer.java:30)
scm_1 | at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:732)
scm_1 | at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:770)
scm_1 | at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:183)
scm_1 | at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:732)
scm_1 | at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:770)
scm_1 | at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:183)
scm_1 | at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:732)
scm_1 | at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:770)
scm_1 | at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:183)
scm_1 | at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider._serialize(DefaultSerializerProvider.java:502)
scm_1 | at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:341)
scm_1 | at com.fasterxml.jackson.databind.ObjectMapper._writeValueAndClose(ObjectMapper.java:4793)
scm_1 | at com.fasterxml.jackson.databind.ObjectMapper.writeValueAsString(ObjectMapper.java:4038)
scm_1 | at org.apache.hadoop.hdds.server.JsonUtils.toJsonString(JsonUtils.java:70)
scm_1 | at org.apache.hadoop.hdds.server.events.EventQueue.serializeObject(EventQueue.java:75)
scm_1 | at org.apache.hadoop.hdds.server.events.EventQueue.fireEvent(EventQueue.java:195)
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -205,11 +192,14 @@ public <PAYLOAD, EVENT_TYPE extends Event<PAYLOAD>> void fireEvent(
for (EventHandler handler : executorAndHandlers.getValue()) {
queuedCount.incrementAndGet();
if (LOG.isTraceEnabled()) {
- LOG.trace(
- "Delivering [event={}] to executor/handler {}: <json>{}</json>",
- event.getName(),
- executorAndHandlers.getKey().getName(),
- TRACING_SERIALIZER.toJson(payload).replaceAll("\n", "\\\\n"));
+ String jsonPayload = serializeObject(payload);
+ if (jsonPayload != null) {
+ LOG.trace(
+ "Delivering [event={}] to executor/handler {}: <json>{}</json>",
+ event.getName(),
+ executorAndHandlers.getKey().getName(),
+ jsonPayload.replaceAll("\n", "\\\\n"));
+ }
Review Comment:
If serialization fails, let's still log it without the payload, at debug level (using the `else if` branch).
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -79,17 +70,13 @@ public EventQueue(String threadNamePrefix) {
// The field parent in DatanodeDetails class has the circular reference
// which will result in Gson infinite recursive parsing. We need to exclude
// this field when generating json string for DatanodeDetails object
Review Comment:
Comment seems to be outdated.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerMetadataInspector.java:
##########
@@ -376,48 +367,42 @@ private void checkJsonReportForIncorrectContainer(JsonObject inspectJson,
}
private void checkJsonErrorsReport(
- JsonObject jsonReport, String propertyValue,
+ JsonNode jsonReport, String propertyValue,
long correctExpected, long correctActual,
boolean correctRepair) {
if (correctExpected == correctActual) {
return;
}
- checkJsonErrorsReport(jsonReport, propertyValue,
- new JsonPrimitive(correctExpected),
- new JsonPrimitive(correctActual),
- correctRepair);
+ JsonNode correctExpectedNode = JsonUtils.valueToJsonNode(correctExpected);
+ JsonNode correctActualNode = JsonUtils.valueToJsonNode(correctActual);
+
+ checkJsonErrorsReport(jsonReport, propertyValue, correctExpectedNode,
+ correctActualNode, correctRepair);
}
/**
* Checks the erorr list in the provided JsonReport for an error matching
* the template passed in with the parameters.
*/
- private void checkJsonErrorsReport(JsonObject jsonReport,
- String propertyValue, JsonPrimitive correctExpected,
- JsonPrimitive correctActual, boolean correctRepair) {
+ private void checkJsonErrorsReport(JsonNode jsonReport,
+ String propertyValue,
+ JsonNode correctExpected,
+ JsonNode correctActual,
+ boolean correctRepair) {
Review Comment:
Let's keep original formatting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1586542348
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/server/JsonUtils.java:
##########
@@ -79,14 +81,18 @@ public static ObjectNode createObjectNode(Object next) {
return MAPPER.valueToTree(next);
}
+ public static JsonNode valueToJsonNode(Object value) {
+ return MAPPER.valueToTree(value);
+ }
+
public static JsonNode readTree(String content) throws IOException {
return MAPPER.readTree(content);
}
- public static List<HashMap<String, Object>> readTreeAsListOfMaps(String json)
+ public static ArrayList<LinkedHashMap<String, Object>> readTreeAsListOfMaps(String json)
Review Comment:
We should replace `ArrayList` with `List` with `ArrayList` but the other way.
BTW, this method is only used in tests. Create a `JsonTestUtils` in `src/test` and then move the test methods there..
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/server/JsonUtils.java:
##########
@@ -79,14 +81,18 @@ public static ObjectNode createObjectNode(Object next) {
return MAPPER.valueToTree(next);
}
+ public static JsonNode valueToJsonNode(Object value) {
Review Comment:
Move it to `src/test`.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/server/JsonUtils.java:
##########
@@ -116,4 +122,26 @@ public static <T> List<T> readFromFile(File file, Class<T> itemType)
}
}
+ /**
+ * Reads JSON content from a Reader and deserializes it into an array of the
+ * specified type.
+ */
+ public static <T> T[] readArrayFromReader(Reader reader, Class<T[]> valueType)
+ throws IOException {
+ return MAPPER.readValue(reader, valueType);
+ }
+
+ /**
+ * Converts a JsonNode into a Java object of the specified type.
+ * @param node The JsonNode to convert.
+ * @param valueType The target class of the Java object.
+ * @param <T> The type of the Java object.
+ * @return A Java object of type T, populated with data from the JsonNode.
+ * @throws IOException
+ */
+ public static <T> T treeToValue(JsonNode node, Class<T> valueType)
Review Comment:
Move it to test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1587362135
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,61 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
- boolean correct = checkAndRepair(containerJson, kvData, store);
-
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ ObjectNode containerJson = inspectContainer(kvData, store);
+ boolean correct = checkAndRepair(containerJson, kvData,
+ store);
+
+ ObjectMapper objectMapper = new ObjectMapper();
+ objectMapper.enable(
+ SerializationFeature.INDENT_OUTPUT);
+ String jsonReport = null;
+ try {
+ jsonReport = objectMapper.writeValueAsString(containerJson);
Review Comment:
> Thanks for the comment @devmadhuu
>
> We are applying `objectMapper.enable(SerializationFeature.INDENT_OUTPUT);` to enable the `INDENT_OUTPUT` feature of the SerializationFeature class. This feature instructs the ObjectMapper to include indentation (line breaks and whitespace) when serializing Java objects to JSON. The shared objectMapper instance of JsonUtils does not have this serialization feature enabled because it is required in some places but not others. Since this ObjectMapper instance is shared among various classes, I thought it would be better to create a separate ObjectMapper instance for this class to enable the indentation feature.
So this INDENT_OUTPUT may be needed at multiple places in future, so its better to have it at common place in JsonUtils
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1590250270
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -373,33 +375,32 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject blockCountError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode blockCountError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.BLOCK_COUNT, blockCountAggregate, blockCountDB,
keyRepairAction);
errors.add(blockCountError);
}
// Check and repair used bytes.
- JsonElement usedBytesDB = parent.getAsJsonObject("dBMetadata")
- .get(OzoneConsts.CONTAINER_BYTES_USED);
- JsonElement usedBytesAggregate = parent.getAsJsonObject("aggregates")
- .get("usedBytes");
+ JsonNode usedBytesDB = parent.path("dBMetadata")
+ .path(OzoneConsts.CONTAINER_BYTES_USED);
+ JsonNode usedBytesAggregate = parent.path("aggregates").path("usedBytes");
// If used bytes is absent from the DB, it is only an error if there is
// a non-zero aggregate of used bytes among the block keys.
long usedBytesDBLong = 0;
- if (!usedBytesDB.isJsonNull()) {
- usedBytesDBLong = usedBytesDB.getAsLong();
+ if (!usedBytesDB.isMissingNode()) {
Review Comment:
Thanks @devmadhuu for raising this point.
We should not be using `isMissingNode()` for jackson code, rather we should be using `isNull()` which is the equivalent for `isJsonNull` of GSON.
Jackson's `isNull()`:
- Similar to Gson's `isJsonNull()`, this checks if the JsonNode represents a null value explicitly set in the JSON.
- Example: For a JSON with "key": null, calling get("key") (or path("key")) and then `isNull()` on the resulting node will return true.
I will make the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567092750
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -463,32 +469,37 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject chunksDirError = buildErrorAndRepair("chunksDirectory.present",
- new JsonPrimitive(true), chunksDirPresent, dirRepairAction);
- errors.add(chunksDirError);
+ ObjectNode chunksDirError =
+ buildErrorAndRepair("chunksDirectory.present",
+ JsonNodeFactory.instance.booleanNode(true), chunksDirPresent,
+ dirRepairAction);
+ ((ArrayNode) errors).add(
+ chunksDirError); // Assuming 'errors' is an ArrayNode
Review Comment:
Changed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1587363239
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -373,33 +375,32 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject blockCountError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode blockCountError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.BLOCK_COUNT, blockCountAggregate, blockCountDB,
keyRepairAction);
errors.add(blockCountError);
}
// Check and repair used bytes.
- JsonElement usedBytesDB = parent.getAsJsonObject("dBMetadata")
- .get(OzoneConsts.CONTAINER_BYTES_USED);
- JsonElement usedBytesAggregate = parent.getAsJsonObject("aggregates")
- .get("usedBytes");
+ JsonNode usedBytesDB = parent.path("dBMetadata")
+ .path(OzoneConsts.CONTAINER_BYTES_USED);
+ JsonNode usedBytesAggregate = parent.path("aggregates").path("usedBytes");
// If used bytes is absent from the DB, it is only an error if there is
// a non-zero aggregate of used bytes among the block keys.
long usedBytesDBLong = 0;
- if (!usedBytesDB.isJsonNull()) {
- usedBytesDBLong = usedBytesDB.getAsLong();
+ if (!usedBytesDB.isMissingNode()) {
Review Comment:
> The `isJsonNull()` method is used to check if the JsonElement representing `usedBytesDB` is a JSON null value. If it's not null, then the value is extracted using getAsLong().
>
> The `isMissingNode()` method is used to check if the JsonNode representing `usedBytesDB` is missing or not present in the JSON structure. If it's not missing, then the value is extracted using asLong().
Pls check old code for `blockCountDB`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6500:
URL: https://github.com/apache/ozone/pull/6500#issuecomment-2087995516
@ArafatKhan2198 Let's remove changes related to `EventQueue` from this patch, and address that in a follow-up task.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1590254911
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,61 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
- boolean correct = checkAndRepair(containerJson, kvData, store);
-
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ ObjectNode containerJson = inspectContainer(kvData, store);
+ boolean correct = checkAndRepair(containerJson, kvData,
+ store);
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567090630
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,63 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
- boolean correct = checkAndRepair(containerJson, kvData, store);
-
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ // Assuming inspectContainer method returns an ObjectNode instead of JsonObject
+ // You will need to refactor inspectContainer to construct an ObjectNode using Jackson
Review Comment:
Removed thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567095949
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -79,17 +70,13 @@ public EventQueue(String threadNamePrefix) {
// The field parent in DatanodeDetails class has the circular reference
// which will result in Gson infinite recursive parsing. We need to exclude
// this field when generating json string for DatanodeDetails object
Review Comment:
Removed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567337156
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -79,17 +70,13 @@ public EventQueue(String threadNamePrefix) {
// The field parent in DatanodeDetails class has the circular reference
// which will result in Gson infinite recursive parsing. We need to exclude
// this field when generating json string for DatanodeDetails object
- static class DatanodeDetailsGsonExclusionStrategy
- implements ExclusionStrategy {
- @Override
- public boolean shouldSkipField(FieldAttributes f) {
- return f.getDeclaringClass() == NodeImpl.class
- && f.getName().equals("parent");
- }
-
- @Override
- public boolean shouldSkipClass(Class<?> aClass) {
- return false;
+ public String serializeObject(Object obj) {
+ try {
+ return JsonUtils.toJsonString(obj);
+ } catch (Exception e) {
+ // Handle the exception, maybe log it
+ LOG.error("Error serializing object: ", e);
Review Comment:
Please try:
```
ozone insight --verbose logs -v scm.event-queue
```
to see if it provides stack trace for the message (`No value present` is likely to come from some `Optional` without value).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1588944996
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/container/common/helpers/DeletedBlocksTransactionInfoWrapper.java:
##########
@@ -38,6 +38,12 @@ public DeletedBlocksTransactionInfoWrapper(long txID, long containerID,
this.localIdList = localIdList;
this.count = count;
}
+ public DeletedBlocksTransactionInfoWrapper() {
Review Comment:
6883aee5ee02825d356ac9a8050312eafdb1b69a removed `ResetDeletedBlockRetryCountSubcommand` from the PR, maybe accidentally. This new `DeletedBlocksTransactionInfoWrapper` constructor belongs to that part.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java:
##########
@@ -144,18 +143,18 @@ public BadFormatException(String msg) {
}
private void processConfigTagRequest(HttpServletRequest request, String cmd,
- Writer out) throws IOException {
- Gson gson = new Gson();
+ Writer out) throws IOException {
Review Comment:
nit: please avoid unnecessary whitespace change (also, such indentation triggers unnecessary changes, e.g. when method is renamed or visibility updated)
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,61 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
- boolean correct = checkAndRepair(containerJson, kvData, store);
-
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ ObjectNode containerJson = inspectContainer(kvData, store);
+ boolean correct = checkAndRepair(containerJson, kvData,
+ store);
Review Comment:
nit: please avoid unrelated formatting change
```suggestion
boolean correct = checkAndRepair(containerJson, kvData, store);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1590356591
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -373,33 +375,32 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject blockCountError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode blockCountError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.BLOCK_COUNT, blockCountAggregate, blockCountDB,
keyRepairAction);
errors.add(blockCountError);
}
// Check and repair used bytes.
- JsonElement usedBytesDB = parent.getAsJsonObject("dBMetadata")
- .get(OzoneConsts.CONTAINER_BYTES_USED);
- JsonElement usedBytesAggregate = parent.getAsJsonObject("aggregates")
- .get("usedBytes");
+ JsonNode usedBytesDB = parent.path("dBMetadata")
+ .path(OzoneConsts.CONTAINER_BYTES_USED);
+ JsonNode usedBytesAggregate = parent.path("aggregates").path("usedBytes");
// If used bytes is absent from the DB, it is only an error if there is
// a non-zero aggregate of used bytes among the block keys.
long usedBytesDBLong = 0;
- if (!usedBytesDB.isJsonNull()) {
- usedBytesDBLong = usedBytesDB.getAsLong();
+ if (!usedBytesDB.isMissingNode()) {
Review Comment:
Sure, will wait for your change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567094853
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -79,17 +70,13 @@ public EventQueue(String threadNamePrefix) {
// The field parent in DatanodeDetails class has the circular reference
// which will result in Gson infinite recursive parsing. We need to exclude
// this field when generating json string for DatanodeDetails object
- static class DatanodeDetailsGsonExclusionStrategy
- implements ExclusionStrategy {
- @Override
- public boolean shouldSkipField(FieldAttributes f) {
- return f.getDeclaringClass() == NodeImpl.class
- && f.getName().equals("parent");
- }
-
- @Override
- public boolean shouldSkipClass(Class<?> aClass) {
- return false;
+ public String serializeObject(Object obj) {
+ try {
+ return JsonUtils.toJsonString(obj);
+ } catch (Exception e) {
+ // Handle the exception, maybe log it
+ LOG.error("Error serializing object: ", e);
Review Comment:
These are the steps I followed to replicate:-
1. Started an unsecure ozone cluster on docker
2. Ran a freon command to create multiple files
3. While the files were getting created I executed the command ozone insight logs -v scm.event-queue
But I got No value present for it hence no parsing of any sort happened hence I could not see the error in SCM logs
Do you know how I can replicate it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1573280367
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -204,12 +187,22 @@ public <PAYLOAD, EVENT_TYPE extends Event<PAYLOAD>> void fireEvent(
for (EventHandler handler : executorAndHandlers.getValue()) {
queuedCount.incrementAndGet();
- if (LOG.isTraceEnabled()) {
+ String jsonPayload = null;
+
+ try {
+ jsonPayload = serializeObject(payload);
Review Comment:
This serializes the JSON even if log level is neither debug nor trace.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -79,17 +70,13 @@ public EventQueue(String threadNamePrefix) {
// The field parent in DatanodeDetails class has the circular reference
// which will result in Gson infinite recursive parsing. We need to exclude
// this field when generating json string for DatanodeDetails object
- static class DatanodeDetailsGsonExclusionStrategy
- implements ExclusionStrategy {
- @Override
- public boolean shouldSkipField(FieldAttributes f) {
- return f.getDeclaringClass() == NodeImpl.class
- && f.getName().equals("parent");
- }
-
- @Override
- public boolean shouldSkipClass(Class<?> aClass) {
- return false;
+ public String serializeObject(Object obj) {
+ try {
+ return JsonUtils.toJsonString(obj);
+ } catch (Exception e) {
+ // Handle the exception, maybe log it
+ LOG.error("Error serializing object: ", e);
Review Comment:
With the current patch the exception happens even without running `ozone insight`. (See my other comment for the reason.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #6500:
URL: https://github.com/apache/ozone/pull/6500#issuecomment-2096254631
@ArafatKhan2198 , thanks for your contribution!
@devmadhuu , thanks a lot for reviewing this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1590381328
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -373,33 +375,32 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject blockCountError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode blockCountError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.BLOCK_COUNT, blockCountAggregate, blockCountDB,
keyRepairAction);
errors.add(blockCountError);
}
// Check and repair used bytes.
- JsonElement usedBytesDB = parent.getAsJsonObject("dBMetadata")
- .get(OzoneConsts.CONTAINER_BYTES_USED);
- JsonElement usedBytesAggregate = parent.getAsJsonObject("aggregates")
- .get("usedBytes");
+ JsonNode usedBytesDB = parent.path("dBMetadata")
+ .path(OzoneConsts.CONTAINER_BYTES_USED);
+ JsonNode usedBytesAggregate = parent.path("aggregates").path("usedBytes");
// If used bytes is absent from the DB, it is only an error if there is
// a non-zero aggregate of used bytes among the block keys.
long usedBytesDBLong = 0;
- if (!usedBytesDB.isJsonNull()) {
- usedBytesDBLong = usedBytesDB.getAsLong();
+ if (!usedBytesDB.isMissingNode()) {
Review Comment:
Appologies @szetszwo I missed it in my previous commit, I have made the changes now please do take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1587290041
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -373,33 +375,32 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject blockCountError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode blockCountError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.BLOCK_COUNT, blockCountAggregate, blockCountDB,
keyRepairAction);
errors.add(blockCountError);
}
// Check and repair used bytes.
- JsonElement usedBytesDB = parent.getAsJsonObject("dBMetadata")
- .get(OzoneConsts.CONTAINER_BYTES_USED);
- JsonElement usedBytesAggregate = parent.getAsJsonObject("aggregates")
- .get("usedBytes");
+ JsonNode usedBytesDB = parent.path("dBMetadata")
+ .path(OzoneConsts.CONTAINER_BYTES_USED);
+ JsonNode usedBytesAggregate = parent.path("aggregates").path("usedBytes");
// If used bytes is absent from the DB, it is only an error if there is
// a non-zero aggregate of used bytes among the block keys.
long usedBytesDBLong = 0;
- if (!usedBytesDB.isJsonNull()) {
- usedBytesDBLong = usedBytesDB.getAsLong();
+ if (!usedBytesDB.isMissingNode()) {
Review Comment:
The `isJsonNull()` method is used to check if the JsonElement representing `usedBytesDB` is a JSON null value. If it's not null, then the value is extracted using getAsLong().
The `isMissingNode()` method is used to check if the JsonNode representing `usedBytesDB` is missing or not present in the JSON structure. If it's not missing, then the value is extracted using asLong().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1590175491
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +180,53 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
+ ObjectNode containerJson = inspectContainer(kvData, store);
boolean correct = checkAndRepair(containerJson, kvData, store);
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
+ String jsonReport = null;
+ jsonReport = JsonUtils.toJsonStringWIthIndent(containerJson);
if (log != null) {
if (correct) {
log.trace(jsonReport);
} else {
log.error(jsonReport);
}
}
+
return jsonReport;
}
- static JsonObject inspectContainer(KeyValueContainerData containerData,
+ static ObjectNode inspectContainer(KeyValueContainerData containerData,
DatanodeStore store) {
- JsonObject containerJson = new JsonObject();
+ ObjectNode containerJson = JsonUtils.createObjectNode(null);
try {
// Build top level container properties.
- containerJson.addProperty("containerID", containerData.getContainerID());
+ containerJson.put("containerID", containerData.getContainerID());
String schemaVersion = containerData.getSchemaVersion();
- containerJson.addProperty("schemaVersion", schemaVersion);
- containerJson.addProperty("containerState",
- containerData.getState().toString());
- containerJson.addProperty("currentDatanodeID",
+ containerJson.put("schemaVersion", schemaVersion);
+ containerJson.put("containerState", containerData.getState().toString());
+ containerJson.put("currentDatanodeID",
containerData.getVolume().getDatanodeUuid());
- containerJson.addProperty("originDatanodeID",
- containerData.getOriginNodeId());
+ containerJson.put("originDatanodeID", containerData.getOriginNodeId());
// Build DB metadata values.
- Table<String, Long> metadataTable = store.getMetadataTable();
- JsonObject dBMetadata = getDBMetadataJson(metadataTable, containerData);
- containerJson.add("dBMetadata", dBMetadata);
+ // Assuming getDBMetadataJson and getAggregateValues methods return ObjectNode and are refactored to use Jackson
+ ObjectNode dBMetadata =
+ getDBMetadataJson(store.getMetadataTable(), containerData);
Review Comment:
Use single line.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java:
##########
@@ -28,13 +28,12 @@
import java.util.Map;
import java.util.Properties;
-import com.google.common.base.Strings;
Review Comment:
It is good to remove the common Strings import. 👍
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -373,33 +365,32 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject blockCountError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode blockCountError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.BLOCK_COUNT, blockCountAggregate, blockCountDB,
keyRepairAction);
errors.add(blockCountError);
}
// Check and repair used bytes.
- JsonElement usedBytesDB = parent.getAsJsonObject("dBMetadata")
- .get(OzoneConsts.CONTAINER_BYTES_USED);
- JsonElement usedBytesAggregate = parent.getAsJsonObject("aggregates")
- .get("usedBytes");
+ JsonNode usedBytesDB = parent.path("dBMetadata")
+ .path(OzoneConsts.CONTAINER_BYTES_USED);
Review Comment:
Use single line.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +180,53 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
+ ObjectNode containerJson = inspectContainer(kvData, store);
boolean correct = checkAndRepair(containerJson, kvData, store);
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
+ String jsonReport = null;
+ jsonReport = JsonUtils.toJsonStringWIthIndent(containerJson);
Review Comment:
Use single line.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -463,32 +453,34 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject chunksDirError = buildErrorAndRepair("chunksDirectory.present",
- new JsonPrimitive(true), chunksDirPresent, dirRepairAction);
+ ObjectNode chunksDirError =
+ buildErrorAndRepair("chunksDirectory.present",
+ JsonNodeFactory.instance.booleanNode(true), chunksDirPresent,
+ dirRepairAction);
Review Comment:
Reformat this as
```java
ObjectNode chunksDirError = buildErrorAndRepair("chunksDirectory.present",
JsonNodeFactory.instance.booleanNode(true), chunksDirPresent, dirRepairAction);
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +180,53 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
+ ObjectNode containerJson = inspectContainer(kvData, store);
boolean correct = checkAndRepair(containerJson, kvData, store);
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
+ String jsonReport = null;
+ jsonReport = JsonUtils.toJsonStringWIthIndent(containerJson);
if (log != null) {
if (correct) {
log.trace(jsonReport);
} else {
log.error(jsonReport);
}
}
+
return jsonReport;
}
- static JsonObject inspectContainer(KeyValueContainerData containerData,
+ static ObjectNode inspectContainer(KeyValueContainerData containerData,
DatanodeStore store) {
- JsonObject containerJson = new JsonObject();
+ ObjectNode containerJson = JsonUtils.createObjectNode(null);
try {
// Build top level container properties.
- containerJson.addProperty("containerID", containerData.getContainerID());
+ containerJson.put("containerID", containerData.getContainerID());
String schemaVersion = containerData.getSchemaVersion();
- containerJson.addProperty("schemaVersion", schemaVersion);
- containerJson.addProperty("containerState",
- containerData.getState().toString());
- containerJson.addProperty("currentDatanodeID",
+ containerJson.put("schemaVersion", schemaVersion);
+ containerJson.put("containerState", containerData.getState().toString());
+ containerJson.put("currentDatanodeID",
containerData.getVolume().getDatanodeUuid());
- containerJson.addProperty("originDatanodeID",
- containerData.getOriginNodeId());
+ containerJson.put("originDatanodeID", containerData.getOriginNodeId());
// Build DB metadata values.
- Table<String, Long> metadataTable = store.getMetadataTable();
- JsonObject dBMetadata = getDBMetadataJson(metadataTable, containerData);
- containerJson.add("dBMetadata", dBMetadata);
+ // Assuming getDBMetadataJson and getAggregateValues methods return ObjectNode and are refactored to use Jackson
+ ObjectNode dBMetadata =
+ getDBMetadataJson(store.getMetadataTable(), containerData);
+ containerJson.set("dBMetadata", dBMetadata);
// Build aggregate values.
- JsonObject aggregates = getAggregateValues(store,
- containerData, schemaVersion);
- containerJson.add("aggregates", aggregates);
+ ObjectNode aggregates =
+ getAggregateValues(store, containerData, schemaVersion);
Review Comment:
Use single line.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -373,33 +375,32 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject blockCountError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode blockCountError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.BLOCK_COUNT, blockCountAggregate, blockCountDB,
keyRepairAction);
errors.add(blockCountError);
}
// Check and repair used bytes.
- JsonElement usedBytesDB = parent.getAsJsonObject("dBMetadata")
- .get(OzoneConsts.CONTAINER_BYTES_USED);
- JsonElement usedBytesAggregate = parent.getAsJsonObject("aggregates")
- .get("usedBytes");
+ JsonNode usedBytesDB = parent.path("dBMetadata")
+ .path(OzoneConsts.CONTAINER_BYTES_USED);
+ JsonNode usedBytesAggregate = parent.path("aggregates").path("usedBytes");
// If used bytes is absent from the DB, it is only an error if there is
// a non-zero aggregate of used bytes among the block keys.
long usedBytesDBLong = 0;
- if (!usedBytesDB.isJsonNull()) {
- usedBytesDBLong = usedBytesDB.getAsLong();
+ if (!usedBytesDB.isMissingNode()) {
Review Comment:
> ... If it's not missing, then the value is extracted using asLong().
Is it possible that it is not missing but equals to null?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -408,18 +399,18 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject usedBytesError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode usedBytesError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.CONTAINER_BYTES_USED, usedBytesAggregate, usedBytesDB,
keyRepairAction);
errors.add(usedBytesError);
}
// check and repair if db delete count mismatches delete transaction count.
- final JsonElement pendingDeleteCountDB = dBMetadata.get(
+ JsonNode pendingDeleteCountDB = dBMetadata.path(
OzoneConsts.PENDING_DELETE_BLOCK_COUNT);
final long dbDeleteCount = jsonToLong(pendingDeleteCountDB);
- final JsonElement pendingDeleteCountAggregate
- = aggregates.get(PendingDelete.COUNT);
+ final JsonNode pendingDeleteCountAggregate
+ = aggregates.path(PendingDelete.COUNT);
Review Comment:
Use single line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567324605
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -79,17 +70,13 @@ public EventQueue(String threadNamePrefix) {
// The field parent in DatanodeDetails class has the circular reference
// which will result in Gson infinite recursive parsing. We need to exclude
// this field when generating json string for DatanodeDetails object
- static class DatanodeDetailsGsonExclusionStrategy
- implements ExclusionStrategy {
- @Override
- public boolean shouldSkipField(FieldAttributes f) {
- return f.getDeclaringClass() == NodeImpl.class
- && f.getName().equals("parent");
- }
-
- @Override
- public boolean shouldSkipClass(Class<?> aClass) {
- return false;
+ public String serializeObject(Object obj) {
+ try {
+ return JsonUtils.toJsonString(obj);
+ } catch (Exception e) {
+ // Handle the exception, maybe log it
+ LOG.error("Error serializing object: ", e);
Review Comment:
This is coming when I execute it inside a docker container running SCM
```
sh-4.2$ ozone insight logs -v scm.event-queue
No value present
sh-4.2$
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567317251
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/events/EventQueue.java:
##########
@@ -79,17 +70,13 @@ public EventQueue(String threadNamePrefix) {
// The field parent in DatanodeDetails class has the circular reference
// which will result in Gson infinite recursive parsing. We need to exclude
// this field when generating json string for DatanodeDetails object
- static class DatanodeDetailsGsonExclusionStrategy
- implements ExclusionStrategy {
- @Override
- public boolean shouldSkipField(FieldAttributes f) {
- return f.getDeclaringClass() == NodeImpl.class
- && f.getName().equals("parent");
- }
-
- @Override
- public boolean shouldSkipClass(Class<?> aClass) {
- return false;
+ public String serializeObject(Object obj) {
+ try {
+ return JsonUtils.toJsonString(obj);
+ } catch (Exception e) {
+ // Handle the exception, maybe log it
+ LOG.error("Error serializing object: ", e);
Review Comment:
Where is `No value present` coming from?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567089022
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/heatmap/TestHeatMapInfo.java:
##########
@@ -745,35 +743,43 @@ public void setUp() throws Exception {
public void testHeatMapGeneratedInfo() throws IOException {
// Setup
// Run the test
- JsonElement jsonElement = JsonParser.parseString(auditRespStr);
- JsonObject jsonObject = jsonElement.getAsJsonObject();
- JsonElement facets = jsonObject.get("facets");
- JsonObject facetsBucketsObject =
- facets.getAsJsonObject().get("resources")
- .getAsJsonObject();
- ObjectMapper objectMapper = new ObjectMapper();
+ // Parse the JSON string to JsonNode
+ JsonNode rootNode = JsonUtils.readTree(auditRespStr);
- HeatMapProviderDataResource auditLogFacetsResources =
- objectMapper.readValue(
- facetsBucketsObject.toString(), HeatMapProviderDataResource.class);
- EntityMetaData[] entities = auditLogFacetsResources.getMetaDataList();
- List<EntityMetaData> entityMetaDataList =
- Arrays.stream(entities).collect(Collectors.toList());
- EntityReadAccessHeatMapResponse entityReadAccessHeatMapResponse =
- heatMapUtil.generateHeatMap(entityMetaDataList);
- assertThat(entityReadAccessHeatMapResponse.getChildren().size()).isGreaterThan(0);
- assertEquals(12, entityReadAccessHeatMapResponse.getChildren().size());
- assertEquals(25600, entityReadAccessHeatMapResponse.getSize());
- assertEquals(2924, entityReadAccessHeatMapResponse.getMinAccessCount());
- assertEquals(155074, entityReadAccessHeatMapResponse.getMaxAccessCount());
- assertEquals("root", entityReadAccessHeatMapResponse.getLabel());
- assertEquals(0.0, entityReadAccessHeatMapResponse.getChildren().get(0).getColor());
- assertEquals(0.442,
- entityReadAccessHeatMapResponse.getChildren().get(0).getChildren()
- .get(0).getChildren().get(1).getColor());
- assertEquals(0.058,
- entityReadAccessHeatMapResponse.getChildren().get(0).getChildren()
- .get(1).getChildren().get(3).getColor());
+ JsonNode facetsNode = rootNode.path("facets");
+ JsonNode resourcesNode = facetsNode.path("resources");
+
+ // Deserialize the resources node directly if it's not missing
+ HeatMapProviderDataResource auditLogFacetsResources = null;
+
+ if (!resourcesNode.isMissingNode()) {
+ auditLogFacetsResources = JsonUtils.treeToValue(resourcesNode,
+ HeatMapProviderDataResource.class);
+ }
+
+ if (auditLogFacetsResources != null) {
Review Comment:
Added the assertion thanks!
The checks for missing nodes are performed using !isMissingNode() instead of checking if the value is not `JsonNull`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567092500
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -437,17 +444,16 @@ private boolean checkAndRepair(JsonObject parent,
return false;
};
- final JsonObject deleteCountError = buildErrorAndRepair(
- "dBMetadata." + OzoneConsts.PENDING_DELETE_BLOCK_COUNT,
- pendingDeleteCountAggregate, pendingDeleteCountDB,
+ final ObjectNode deleteCountError = buildErrorAndRepair("dBMetadata." + OzoneConsts.PENDING_DELETE_BLOCK_COUNT,
Review Comment:
Changed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1567091128
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -165,7 +166,7 @@ public void process(ContainerData containerData, DatanodeStore store) {
}
public String process(ContainerData containerData, DatanodeStore store,
- Logger log) {
+ Logger log) {
Review Comment:
I have removed them from wherever I could find them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #6500:
URL: https://github.com/apache/ozone/pull/6500
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1587207892
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,61 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
- boolean correct = checkAndRepair(containerJson, kvData, store);
-
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ ObjectNode containerJson = inspectContainer(kvData, store);
+ boolean correct = checkAndRepair(containerJson, kvData,
+ store);
+
+ ObjectMapper objectMapper = new ObjectMapper();
+ objectMapper.enable(
+ SerializationFeature.INDENT_OUTPUT);
+ String jsonReport = null;
+ try {
+ jsonReport = objectMapper.writeValueAsString(containerJson);
Review Comment:
Can we use `org.apache.hadoop.hdds.server.JsonUtils#toJsonString` here ?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -373,33 +375,32 @@ private boolean checkAndRepair(JsonObject parent,
return repaired;
};
- JsonObject blockCountError = buildErrorAndRepair("dBMetadata." +
+ ObjectNode blockCountError = buildErrorAndRepair("dBMetadata." +
OzoneConsts.BLOCK_COUNT, blockCountAggregate, blockCountDB,
keyRepairAction);
errors.add(blockCountError);
}
// Check and repair used bytes.
- JsonElement usedBytesDB = parent.getAsJsonObject("dBMetadata")
- .get(OzoneConsts.CONTAINER_BYTES_USED);
- JsonElement usedBytesAggregate = parent.getAsJsonObject("aggregates")
- .get("usedBytes");
+ JsonNode usedBytesDB = parent.path("dBMetadata")
+ .path(OzoneConsts.CONTAINER_BYTES_USED);
+ JsonNode usedBytesAggregate = parent.path("aggregates").path("usedBytes");
// If used bytes is absent from the DB, it is only an error if there is
// a non-zero aggregate of used bytes among the block keys.
long usedBytesDBLong = 0;
- if (!usedBytesDB.isJsonNull()) {
- usedBytesDBLong = usedBytesDB.getAsLong();
+ if (!usedBytesDB.isMissingNode()) {
Review Comment:
`isNull `and i`sMissingNode` functions as same ? Because for `blockCountDB`, you used `isNull` and here for `usedBytesDB`, you have used "`isMissingNode`". You have replaced Old code which was using `isJsonNull`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -412,11 +412,13 @@ private int getNumOfBuckets(String bucketPrefix)
* Parse output into ArrayList with Gson.
* @return ArrayList
*/
- private ArrayList<LinkedTreeMap<String, String>> parseOutputIntoArrayList()
- throws UnsupportedEncodingException {
- return new Gson().fromJson(out.toString(DEFAULT_ENCODING), ArrayList.class);
+ private ArrayList<LinkedHashMap<String, Object>> parseOutputIntoArrayList()
Review Comment:
Is it necessary to use a specific "`ArrayList`" `LinkedHashMap` reference ? can't we use just `List` and `HashMap` ?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -1696,7 +1699,7 @@ public void testListVolumeBucketKeyShouldPrintValidJsonArray()
execute(ozoneShell, new String[] {"volume", "list"});
// Expect valid JSON array
- final ArrayList<LinkedTreeMap<String, String>> volumeListOut =
+ final ArrayList<LinkedHashMap<String, Object>> volumeListOut =
Review Comment:
same as above comment. Pls check all places.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconWithOzoneManager.java:
##########
@@ -383,11 +383,11 @@ private long getReconTaskAttributeFromJson(String taskStatusResponse,
String taskName,
String entityAttribute)
throws IOException {
- List<HashMap<String, Object>> taskStatusList =
+ List<LinkedHashMap<String, Object>> taskStatusList =
Review Comment:
Is it necessary to use a specific `LinkedHashMap` reference ? can't we use `HashMap` ?
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerMetadataInspector.java:
##########
@@ -521,8 +512,11 @@ private JsonObject runInspectorAndGetReport(
capturer.stopCapturing();
String output = capturer.getOutput();
capturer.clearOutput();
-
- return new Gson().fromJson(output, JsonObject.class);
+ // Check if the output is effectively empty
+ if (output.trim().isEmpty()) {
+ return null;
Review Comment:
Since we are returning null in this method, pls make sure no impacts on callers of this method like NPE or other impact.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/admin/scm/ResetDeletedBlockRetryCountSubcommand.java:
##########
@@ -92,9 +90,12 @@ public void execute(ScmClient client) throws IOException {
System.out.println("The last loaded txID: " +
txIDs.get(txIDs.size() - 1));
}
- } catch (JsonIOException | JsonSyntaxException | IOException ex) {
- System.out.println("Cannot parse the file " + group.fileName);
+ } catch (JsonProcessingException ex) {
+ System.out.println("Error parsing JSON: " + ex.getMessage());
throw new IOException(ex);
+ } catch (IOException ex) {
Review Comment:
Can we combine these 2 exception same as old code ? Because `JsonProcessingException` is subclass of IOException and even if JSON parse exception will be thrown, then also you will get correct error info to log as we are wrapping in IOException and then throwing it again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1587276777
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +182,61 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
- boolean correct = checkAndRepair(containerJson, kvData, store);
-
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
- if (log != null) {
- if (correct) {
- log.trace(jsonReport);
- } else {
- log.error(jsonReport);
+ ObjectNode containerJson = inspectContainer(kvData, store);
+ boolean correct = checkAndRepair(containerJson, kvData,
+ store);
+
+ ObjectMapper objectMapper = new ObjectMapper();
+ objectMapper.enable(
+ SerializationFeature.INDENT_OUTPUT);
+ String jsonReport = null;
+ try {
+ jsonReport = objectMapper.writeValueAsString(containerJson);
Review Comment:
Thanks for the comment @devmadhuu
We are applying `objectMapper.enable(SerializationFeature.INDENT_OUTPUT);` to enable the `INDENT_OUTPUT` feature of the SerializationFeature class. This feature instructs the ObjectMapper to include indentation (line breaks and whitespace) when serializing Java objects to JSON. The shared objectMapper instance of JsonUtils does not have this serialization feature enabled because it is required in some places but not others. Since this ObjectMapper instance is shared among various classes, I thought it would be better to create a separate ObjectMapper instance for this class to enable the indentation feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10542. Replace remaining GSON usage with Jackson. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6500:
URL: https://github.com/apache/ozone/pull/6500#discussion_r1590254561
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +180,53 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
+ ObjectNode containerJson = inspectContainer(kvData, store);
boolean correct = checkAndRepair(containerJson, kvData, store);
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
+ String jsonReport = null;
+ jsonReport = JsonUtils.toJsonStringWIthIndent(containerJson);
Review Comment:
Done!
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +180,53 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
+ ObjectNode containerJson = inspectContainer(kvData, store);
boolean correct = checkAndRepair(containerJson, kvData, store);
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
+ String jsonReport = null;
+ jsonReport = JsonUtils.toJsonStringWIthIndent(containerJson);
if (log != null) {
if (correct) {
log.trace(jsonReport);
} else {
log.error(jsonReport);
}
}
+
return jsonReport;
}
- static JsonObject inspectContainer(KeyValueContainerData containerData,
+ static ObjectNode inspectContainer(KeyValueContainerData containerData,
DatanodeStore store) {
- JsonObject containerJson = new JsonObject();
+ ObjectNode containerJson = JsonUtils.createObjectNode(null);
try {
// Build top level container properties.
- containerJson.addProperty("containerID", containerData.getContainerID());
+ containerJson.put("containerID", containerData.getContainerID());
String schemaVersion = containerData.getSchemaVersion();
- containerJson.addProperty("schemaVersion", schemaVersion);
- containerJson.addProperty("containerState",
- containerData.getState().toString());
- containerJson.addProperty("currentDatanodeID",
+ containerJson.put("schemaVersion", schemaVersion);
+ containerJson.put("containerState", containerData.getState().toString());
+ containerJson.put("currentDatanodeID",
containerData.getVolume().getDatanodeUuid());
- containerJson.addProperty("originDatanodeID",
- containerData.getOriginNodeId());
+ containerJson.put("originDatanodeID", containerData.getOriginNodeId());
// Build DB metadata values.
- Table<String, Long> metadataTable = store.getMetadataTable();
- JsonObject dBMetadata = getDBMetadataJson(metadataTable, containerData);
- containerJson.add("dBMetadata", dBMetadata);
+ // Assuming getDBMetadataJson and getAggregateValues methods return ObjectNode and are refactored to use Jackson
+ ObjectNode dBMetadata =
+ getDBMetadataJson(store.getMetadataTable(), containerData);
Review Comment:
Done!
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -181,55 +180,53 @@ public String process(ContainerData containerData, DatanodeStore store,
return null;
}
- JsonObject containerJson = inspectContainer(kvData, store);
+ ObjectNode containerJson = inspectContainer(kvData, store);
boolean correct = checkAndRepair(containerJson, kvData, store);
- Gson gson = new GsonBuilder()
- .setPrettyPrinting()
- .serializeNulls()
- .create();
- String jsonReport = gson.toJson(containerJson);
+ String jsonReport = null;
+ jsonReport = JsonUtils.toJsonStringWIthIndent(containerJson);
if (log != null) {
if (correct) {
log.trace(jsonReport);
} else {
log.error(jsonReport);
}
}
+
return jsonReport;
}
- static JsonObject inspectContainer(KeyValueContainerData containerData,
+ static ObjectNode inspectContainer(KeyValueContainerData containerData,
DatanodeStore store) {
- JsonObject containerJson = new JsonObject();
+ ObjectNode containerJson = JsonUtils.createObjectNode(null);
try {
// Build top level container properties.
- containerJson.addProperty("containerID", containerData.getContainerID());
+ containerJson.put("containerID", containerData.getContainerID());
String schemaVersion = containerData.getSchemaVersion();
- containerJson.addProperty("schemaVersion", schemaVersion);
- containerJson.addProperty("containerState",
- containerData.getState().toString());
- containerJson.addProperty("currentDatanodeID",
+ containerJson.put("schemaVersion", schemaVersion);
+ containerJson.put("containerState", containerData.getState().toString());
+ containerJson.put("currentDatanodeID",
containerData.getVolume().getDatanodeUuid());
- containerJson.addProperty("originDatanodeID",
- containerData.getOriginNodeId());
+ containerJson.put("originDatanodeID", containerData.getOriginNodeId());
// Build DB metadata values.
- Table<String, Long> metadataTable = store.getMetadataTable();
- JsonObject dBMetadata = getDBMetadataJson(metadataTable, containerData);
- containerJson.add("dBMetadata", dBMetadata);
+ // Assuming getDBMetadataJson and getAggregateValues methods return ObjectNode and are refactored to use Jackson
+ ObjectNode dBMetadata =
+ getDBMetadataJson(store.getMetadataTable(), containerData);
+ containerJson.set("dBMetadata", dBMetadata);
// Build aggregate values.
- JsonObject aggregates = getAggregateValues(store,
- containerData, schemaVersion);
- containerJson.add("aggregates", aggregates);
+ ObjectNode aggregates =
+ getAggregateValues(store, containerData, schemaVersion);
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org