Copilot commented on code in PR #11296:
URL: https://github.com/apache/ozone/pull/11296#discussion_r4123957111
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/SnapDiffDependencyEntry.java:
##########
@@ -141,6 +146,59 @@ void clearPathCache() {
targetPathDecoded = false;
}
+ byte[] toResolvedBytes() {
+ String sourcePathStr = new String(reportEntry.getSourcePath(),
StandardCharsets.UTF_8);
+ byte[] sourcePathBytes = sourcePathStr.getBytes(StandardCharsets.UTF_8);
Review Comment:
`toResolvedBytes()` re-decodes then re-encodes `sourcePath` as UTF-8
(`byte[] -> String -> byte[]`). This can (a) change byte content if the
original bytes are not valid UTF-8 (replacement chars), and (b) adds avoidable
allocations/cpu on a hot path. Recommendation (mandatory): write the
`reportEntry.getSourcePath()` bytes directly into the buffer (and likewise
avoid any lossy conversions), keeping the serialization strictly
byte-preserving.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -165,17 +169,19 @@ public class SnapshotDiffManager implements
AutoCloseable, SnapshotDiffManagerMX
private static final Logger LOG =
LoggerFactory.getLogger(SnapshotDiffManager.class);
private static final Map<DiffType, String> DIFF_TYPE_STRING_MAP =
- new EnumMap<>(ImmutableMap.of(DELETE, "1", RENAME, "2", CREATE, "3",
MODIFY, "4"));
+ new EnumMap<>(ImmutableMap.of(DELETE, "1", MODIFY, "2", RENAME, "3",
CREATE, "4"));
Review Comment:
Changing `DIFF_TYPE_STRING_MAP` alters the on-disk encoding of report keys.
If there are any persisted/in-flight diff jobs created with the prior mapping
(e.g., `RENAME=2, CREATE=3, MODIFY=4`), report iteration/parsing can become
inconsistent and may mis-order or misinterpret entries. Recommendation
(mandatory): keep the mapping stable for backward compatibility, or introduce a
versioned key format / migration path so older reports remain readable and
correctly typed/ordered.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/SnapDiffJobStore.java:
##########
@@ -344,11 +788,30 @@ public void close() throws IOException {
flushWrites();
writeBatch.close();
writeOptions.close();
+ if (temporaryColumnFamiliesDropped) {
+ tempColumnFamilyOptions.close();
+ return;
+ }
Review Comment:
`close()` returns early when `temporaryColumnFamiliesDropped` is set,
assuming all job-scoped CF handles have already been dropped/closed elsewhere.
This is fragile: if future refactors set the flag prematurely or skip a drop,
the early return will leak CF handles. Recommendation (mandatory): either (1)
assert all relevant handles are null before returning, or (2) still close any
non-null handles even when `temporaryColumnFamiliesDropped` is true, to make
cleanup robust against partial pipeline execution.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/SnapDiffJobStore.java:
##########
@@ -262,37 +583,131 @@ public void flushWrites() throws IOException {
pendingOps = 0;
}
+ /**
+ * Starts batched emission of resolved rows to the snap diff report table.
+ */
+ public void beginReportWrite() {
+ if (reportCfh == null) {
+ throw new IllegalStateException("Snap diff report column family not
configured for job store");
+ }
+ if (reportWriteStarted) {
+ throw new IllegalStateException("Snap diff report write already started
for job " + jobId);
+ }
+ reportWriteStarted = true;
+ reportIndex = 0;
+ largestReportKey = "";
+ }
+
+ /**
+ * Appends one resolved {@link DiffReportEntry} to the snap diff report
table.
+ */
+ public void putReportEntry(DiffReportEntry entry) throws IOException {
+ if (!reportWriteStarted) {
+ throw new IllegalStateException("Snap diff report write not started for
job " + jobId);
+ }
+ appendReportEntry(entry);
+ }
+
+ /**
+ * Appends a batch of resolved {@link DiffReportEntry}s to the snap diff
report table.
+ */
+ public void putReportEntries(List<DiffReportEntry> entries) throws
IOException {
+ if (!reportWriteStarted) {
+ throw new IllegalStateException("Snap diff report write not started for
job " + jobId);
+ }
+ for (DiffReportEntry entry : entries) {
+ appendReportEntry(entry);
+ }
+ }
+
+ /**
+ * Flushes pending report rows and returns the entry count and largest
report key.
+ */
+ public Pair<Long, String> finishReportWrite() throws IOException {
+ if (!reportWriteStarted) {
+ throw new IllegalStateException("Snap diff report write not started for
job " + jobId);
+ }
+ flushWrites();
+ reportWriteStarted = false;
+ return Pair.of(reportIndex, largestReportKey);
+ }
+
private byte[] objectIdKeyBuffer(long objectId) {
encodeLong(objectIdKeyBuffer, 0, objectId);
return objectIdKeyBuffer;
}
- private byte[] edgeKeyBuffer(long parentId, long objectId) {
- encodeLong(edgeKeyBuffer, 0, parentId);
- encodeLong(edgeKeyBuffer, Long.BYTES, objectId);
- return edgeKeyBuffer;
+ private byte[] intKeyBuffer(int value) {
+ encodeInt(intKeyBuffer, 0, value);
+ return intKeyBuffer;
}
- private static void encodeLong(byte[] buffer, int offset, long value) {
- for (int shift = Long.SIZE - 8; shift >= 0; shift -= 8) {
- buffer[offset++] = (byte) (value >>> shift);
+ private byte[] intValueBuffer(int value) {
+ byte[] buffer = new byte[Integer.BYTES];
+ encodeInt(buffer, 0, value);
+ return buffer;
+ }
Review Comment:
`intValueBuffer()` allocates a new 4-byte array per call. This is used in
dependency-graph persistence (`putDepAdjOffset/Target/InDegree/Order`) and can
become allocation-heavy for large diffs (many nodes/edges). Recommendation
(mandatory for large inputs): reuse a small buffer (similar to `intKeyBuffer`)
where safe, or introduce a pooled/preallocated mechanism for int encoding to
reduce GC pressure during graph 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]
