Copilot commented on code in PR #8981:
URL: https://github.com/apache/ozone/pull/8981#discussion_r2319490193
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java:
##########
@@ -91,21 +106,77 @@ public static boolean verifyChecksum(OmSnapshotLocalData
snapshotData)
snapshotDataCopy.setChecksum(null);
// Get the YAML representation
- final Yaml yaml = getYamlForSnapshotLocalData();
+ try (UncheckedAutoCloseableSupplier<Yaml> yaml =
snapshotManager.getSnapshotLocalYaml()) {
Review Comment:
The verifyChecksum method should handle the IOException that can be thrown
by getSnapshotLocalYaml() instead of letting it propagate as an unchecked
exception. Consider wrapping the IOException in a RuntimeException or adding it
to the method signature.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java:
##########
@@ -91,21 +106,77 @@ public static boolean verifyChecksum(OmSnapshotLocalData
snapshotData)
snapshotDataCopy.setChecksum(null);
// Get the YAML representation
- final Yaml yaml = getYamlForSnapshotLocalData();
+ try (UncheckedAutoCloseableSupplier<Yaml> yaml =
snapshotManager.getSnapshotLocalYaml()) {
+ // Compute new checksum
+ snapshotDataCopy.computeAndSetChecksum(yaml.get());
- // Compute new checksum
- snapshotDataCopy.computeAndSetChecksum(yaml);
+ // Compare the stored and computed checksums
+ String computedChecksum = snapshotDataCopy.getChecksum();
+ boolean isValid = storedChecksum.equals(computedChecksum);
- // Compare the stored and computed checksums
- String computedChecksum = snapshotDataCopy.getChecksum();
- boolean isValid = storedChecksum.equals(computedChecksum);
+ if (!isValid) {
+ LOG.warn("Checksum verification failed for snapshot local data. " +
+ "Stored: {}, Computed: {}", storedChecksum, computedChecksum);
+ }
+ return isValid;
+ }
+ }
+
+ /**
+ * Representer class to define which fields need to be stored in yaml file.
+ */
+ private static class OmSnapshotLocalDataRepresenter extends Representer {
+
+ OmSnapshotLocalDataRepresenter(DumperOptions options) {
+ super(options);
+ this.addClassTag(OmSnapshotLocalDataYaml.class, SNAPSHOT_YAML_TAG);
+ this.addClassTag(VersionMeta.class, SNAPSHOT_VERSION_META_TAG);
+ this.addClassTag(SstFileInfo.class, SST_FILE_INFO_TAG);
+ representers.put(SstFileInfo.class, new RepresentSstFileInfo());
+ representers.put(VersionMeta.class, new RepresentVersionMeta());
+ representers.put(UUID.class, data ->
+ new ScalarNode(Tag.STR, data.toString(), null, null,
DumperOptions.ScalarStyle.PLAIN));
+ }
+
+ private class RepresentSstFileInfo implements Represent {
+ @Override
+ public Node representData(Object data) {
+ SstFileInfo info = (SstFileInfo) data;
+ Map<String, Object> map = new java.util.LinkedHashMap<>();
+ map.put(OzoneConsts.OM_SST_FILE_INFO_FILE_NAME, info.getFileName());
+ map.put(OzoneConsts.OM_SST_FILE_INFO_START_KEY, info.getStartKey());
+ map.put(OzoneConsts.OM_SST_FILE_INFO_END_KEY, info.getEndKey());
+ map.put(OzoneConsts.OM_SST_FILE_INFO_COL_FAMILY,
info.getColumnFamily());
+
+ // Explicitly create a mapping node with the desired tag
+ return representMapping(SST_FILE_INFO_TAG, map,
DumperOptions.FlowStyle.BLOCK);
+ }
+ }
+
+ // New inner class for VersionMeta
+ private class RepresentVersionMeta implements Represent {
+ @Override
+ public Node representData(Object data) {
+ VersionMeta meta = (VersionMeta) data;
+ Map<String, Object> map = new java.util.LinkedHashMap<>();
+ map.put(OzoneConsts.OM_SLD_VERSION_META_PREV_SNAP_VERSION,
meta.getPreviousSnapshotVersion());
+ map.put(OzoneConsts.OM_SLD_VERSION_META_SST_FILES, meta.getSstFiles());
- if (!isValid) {
- LOG.warn("Checksum verification failed for snapshot local data. " +
- "Stored: {}, Computed: {}", storedChecksum, computedChecksum);
+ return representMapping(SNAPSHOT_VERSION_META_TAG, map,
DumperOptions.FlowStyle.BLOCK);
+ }
}
Review Comment:
The cast to VersionMeta on line 159 is unsafe. Consider adding a type check
before casting to prevent ClassCastException if an incorrect object type is
passed.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/compaction/log/CompactionFileInfo.java:
##########
@@ -117,8 +94,25 @@ public static CompactionFileInfo getFromProtobuf(
@Override
public String toString() {
- return String.format("fileName: '%s', startKey: '%s', endKey: '%s'," +
- " columnFamily: '%s', isPruned: '%b'", fileName, startKey, endKey,
columnFamily, pruned);
+ return String.format("%s, isPruned: '%b'", super.toString(), pruned);
+ }
+
+ @Override
+ public SstFileInfo copyObject() {
+ return new CompactionFileInfo(getFileName(), getStartKey(), getEndKey(),
getColumnFamily(), pruned);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof CompactionFileInfo)) {
+ return false;
+ }
+ return super.equals(o) && pruned == ((CompactionFileInfo)o).pruned;
Review Comment:
[nitpick] The cast to CompactionFileInfo is unsafe since the type check was
performed for CompactionFileInfo. However, consider storing the cast result in
a variable for better readability: `CompactionFileInfo other =
(CompactionFileInfo) o; return super.equals(o) && pruned == other.pruned;`
```suggestion
CompactionFileInfo other = (CompactionFileInfo) o;
return super.equals(o) && pruned == other.pruned;
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java:
##########
@@ -91,21 +106,77 @@ public static boolean verifyChecksum(OmSnapshotLocalData
snapshotData)
snapshotDataCopy.setChecksum(null);
// Get the YAML representation
- final Yaml yaml = getYamlForSnapshotLocalData();
+ try (UncheckedAutoCloseableSupplier<Yaml> yaml =
snapshotManager.getSnapshotLocalYaml()) {
+ // Compute new checksum
+ snapshotDataCopy.computeAndSetChecksum(yaml.get());
- // Compute new checksum
- snapshotDataCopy.computeAndSetChecksum(yaml);
+ // Compare the stored and computed checksums
+ String computedChecksum = snapshotDataCopy.getChecksum();
+ boolean isValid = storedChecksum.equals(computedChecksum);
- // Compare the stored and computed checksums
- String computedChecksum = snapshotDataCopy.getChecksum();
- boolean isValid = storedChecksum.equals(computedChecksum);
+ if (!isValid) {
+ LOG.warn("Checksum verification failed for snapshot local data. " +
+ "Stored: {}, Computed: {}", storedChecksum, computedChecksum);
+ }
+ return isValid;
+ }
+ }
+
+ /**
+ * Representer class to define which fields need to be stored in yaml file.
+ */
+ private static class OmSnapshotLocalDataRepresenter extends Representer {
+
+ OmSnapshotLocalDataRepresenter(DumperOptions options) {
+ super(options);
+ this.addClassTag(OmSnapshotLocalDataYaml.class, SNAPSHOT_YAML_TAG);
+ this.addClassTag(VersionMeta.class, SNAPSHOT_VERSION_META_TAG);
+ this.addClassTag(SstFileInfo.class, SST_FILE_INFO_TAG);
+ representers.put(SstFileInfo.class, new RepresentSstFileInfo());
+ representers.put(VersionMeta.class, new RepresentVersionMeta());
+ representers.put(UUID.class, data ->
+ new ScalarNode(Tag.STR, data.toString(), null, null,
DumperOptions.ScalarStyle.PLAIN));
+ }
+
+ private class RepresentSstFileInfo implements Represent {
+ @Override
+ public Node representData(Object data) {
+ SstFileInfo info = (SstFileInfo) data;
+ Map<String, Object> map = new java.util.LinkedHashMap<>();
+ map.put(OzoneConsts.OM_SST_FILE_INFO_FILE_NAME, info.getFileName());
+ map.put(OzoneConsts.OM_SST_FILE_INFO_START_KEY, info.getStartKey());
+ map.put(OzoneConsts.OM_SST_FILE_INFO_END_KEY, info.getEndKey());
+ map.put(OzoneConsts.OM_SST_FILE_INFO_COL_FAMILY,
info.getColumnFamily());
+
+ // Explicitly create a mapping node with the desired tag
+ return representMapping(SST_FILE_INFO_TAG, map,
DumperOptions.FlowStyle.BLOCK);
+ }
+ }
Review Comment:
The cast to SstFileInfo on line 144 is unsafe. Consider adding a type check
before casting to prevent ClassCastException if an incorrect object type is
passed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
