Copilot commented on code in PR #10210:
URL: https://github.com/apache/ozone/pull/10210#discussion_r3210566973
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java:
##########
@@ -183,18 +183,12 @@ public Object construct(Node node) {
// Set other fields from parsed YAML
snapshotLocalData.setSstFiltered((Boolean)
nodes.getOrDefault(OzoneConsts.OM_SLD_IS_SST_FILTERED, false));
-
- // Handle potential Integer/Long type mismatch from YAML parsing
- Object lastDefragTimeObj =
nodes.getOrDefault(OzoneConsts.OM_SLD_LAST_DEFRAG_TIME, -1L);
- long lastDefragTime;
- if (lastDefragTimeObj instanceof Number) {
- lastDefragTime = ((Number) lastDefragTimeObj).longValue();
- } else {
+ Object lastDefragTimeObj =
nodes.getOrDefault(OzoneConsts.OM_SLD_LAST_DEFRAG_TIME, 0L);
+ if (!(lastDefragTimeObj instanceof Number)) {
throw new IllegalArgumentException("Invalid type for lastDefragTime:
" +
lastDefragTimeObj.getClass().getName() + ". Expected Number
type.");
}
- snapshotLocalData.setLastDefragTime(lastDefragTime);
-
+ snapshotLocalData.setLastDefragTime(((Number)
lastDefragTimeObj).longValue());
Review Comment:
`lastDefragTimeObj` can be `null` if the YAML contains `lastDefragTime:`
with no value. In that case the `instanceof Number` check fails and the
exception path dereferences `lastDefragTimeObj.getClass()`, causing a
`NullPointerException` instead of a clear error (or a default). Handle `null`
explicitly (eg treat it as 0L, or throw `IllegalArgumentException` with a
message that doesn't dereference null).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotLocalDataManager.java:
##########
@@ -876,6 +877,7 @@ public void addSnapshotVersion(RDBStore snapshotStore)
throws IOException {
Optional<OmSnapshotLocalData> previousSnapshotLocalData =
getPreviousSnapshotLocalData();
this.getSnapshotLocalData().addVersionSSTFileInfos(sstFiles,
previousSnapshotLocalData.map(OmSnapshotLocalData::getVersion).orElse(0));
+ this.getSnapshotLocalData().setLastDefragTime(Time.now());
// Adding a new snapshot version means it has been defragged thus the
flag needs to be reset.
this.getSnapshotLocalData().setNeedsDefrag(false);
Review Comment:
`lastDefragTime` is being set inside `addSnapshotVersion()` before
`commit()` succeeds. This means callers can observe an updated timestamp on the
in-memory object even if the subsequent `commit()` fails or is never called,
and it also doesn’t precisely match the doc intent of “time when a new
defragged snapshot version is committed”. Consider setting `lastDefragTime`
only after a successful commit (or otherwise clearly tying it to persistence).
--
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]
