Copilot commented on code in PR #9553:
URL: https://github.com/apache/ozone/pull/9553#discussion_r2983983881
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java:
##########
@@ -382,19 +538,21 @@ String getCommitString() {
int opSize = 0;
int discardedCount = 0;
int discardedSize = 0;
+ int delRangeCount = 0;
for (FamilyCache f : name2cache.values()) {
putCount += f.putCount;
delCount += f.delCount;
opSize += f.batchSize;
discardedCount += f.discardedCount;
discardedSize += f.discardedSize;
+ delRangeCount += f.delRangeCount;
}
final int opCount = putCount + delCount;
return String.format(
- "#put=%s, #del=%s, batchSize: %s, discarded: %s, committed: %s",
- putCount, delCount,
+ "#put=%s, #del=%s, #delRange=%s, batchSize: %s, discarded: %s,
committed: %s",
+ putCount, delCount, delRangeCount,
countSize2String(opCount, opSize),
countSize2String(discardedCount, discardedSize),
countSize2String(opCount - discardedCount, opSize - discardedSize));
Review Comment:
`FamilyCache` accounting/logging does not include delete-range ops:
`deleteRange(...)` doesn't update `batchSize`, and `getCommitString()` computes
`opCount` as `putCount + delCount` (excluding `delRangeCount`). This makes
commit stats (and the discarded-changes warning) inaccurate, especially for
batches containing only delete-range operations. Track delete-range sizes in
`batchSize` and include `delRangeCount` in the operation counts used for
logging.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java:
##########
@@ -332,6 +478,11 @@ void delete(CodecBuffer key) {
overwriteIfExists(new DeleteOp(key));
}
+ void deleteRange(byte[] startKey, byte[] endKey) {
+ delRangeCount++;
+ ops.put(opIndex.getAndIncrement(), new DeleteRangeOperation(startKey,
endKey));
Review Comment:
`deleteRange(...)` increments `delRangeCount` but does not update
`batchSize`/discard accounting. This makes `batchSizeDiscardedString()` and the
“discarding changes” warning unreliable for batches that include delete-range
ops. Consider adding the range op’s `totalLength()` to `batchSize` (and/or
tracking delete-range bytes separately) for consistent accounting.
```suggestion
DeleteRangeOperation op = new DeleteRangeOperation(startKey, endKey);
batchSize += op.totalLength();
ops.put(opIndex.getAndIncrement(), op);
```
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestRDBBatchOperation.java:
##########
@@ -51,10 +57,19 @@
import org.rocksdb.RocksDBException;
/**
- * The TestRDBBatchOperation class provides test cases to validate the
functionality of RDB batch operations
- * in a RocksDB-based backend. It verifies the correct behavior of write
operations using batch processing
- * and ensures the integrity of operations like put and delete when performed
in batch mode.
- */
+ * Test class for verifying batch operations with delete ranges using the
+ * RDBBatchOperation and MockedConstruction of ManagedWriteBatch.
+ *
+ * This test class includes:
+ * - Mocking and tracking of operations including put, delete, and delete range
+ * within a batch operation.
+ * - Validation of committed operations using assertions on collected data.
+ * - Ensures that the batch operation interacts correctly with the
+ * RocksDatabase and ColumnFamilyHandle components.
+ *
+ * The test method includes:
+ * 1. Setup of mocked ColumnFamilyHandle and RocksDatabase.ColumnFamily.
+ * 2. Mocking of methods to track operations performed on*/
Review Comment:
The class-level Javadoc is incomplete/inaccurate (it ends mid-sentence at
“performed on” and mentions “MockedConstruction of ManagedWriteBatch” even
though the test uses `TrackingUtilManagedWriteBatchForTesting`). Please
fix/trim the Javadoc so it accurately reflects the current test setup.
```suggestion
* Tests batch operations (including delete ranges) using {@link
RDBBatchOperation}
* together with {@link TrackingUtilManagedWriteBatchForTesting} to track and
* verify the puts, deletes, and delete-range operations applied to RocksDB
* column families.
*/
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java:
##########
@@ -239,6 +250,46 @@ boolean closeImpl() {
}
}
+ /**
+ * Delete range operation to be applied to a {@link ColumnFamily} batch.
+ */
+ private final class DeleteRangeOperation extends Op {
+ private final byte[] startKey;
+ private final byte[] endKey;
+ private final Bytes startKeyBytes;
+ private final Bytes endKeyBytes;
+
+ private DeleteRangeOperation(byte[] startKey, byte[] endKey) {
+ this.startKey = Objects.requireNonNull(startKey, "startKey == null");
+ this.endKey = Objects.requireNonNull(endKey, "endKey == null");
+ this.startKeyBytes = new Bytes(startKey);
+ this.endKeyBytes = new Bytes(endKey);
Review Comment:
`DeleteRangeOperation` keeps references to the caller-provided
`startKey`/`endKey` byte arrays. If those arrays are reused or mutated by the
caller after enqueueing, the batch contents and range matching (`contains`) can
become incorrect. Consider defensively copying the arrays (and using the copies
for both the `byte[]` fields and the `Bytes` wrappers).
```suggestion
Objects.requireNonNull(startKey, "startKey == null");
Objects.requireNonNull(endKey, "endKey == null");
this.startKey = startKey.clone();
this.endKey = endKey.clone();
this.startKeyBytes = new Bytes(this.startKey);
this.endKeyBytes = new Bytes(this.endKey);
```
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestRDBTableStore.java:
##########
@@ -775,4 +776,77 @@ private void populateTable(Table<String, String> table,
}
}
}
+
+ @Test
+ public void batchDeleteWithRange() throws Exception {
+ final Table<byte[], byte[]> testTable = rdbStore.getTable("Fifth");
+ try (BatchOperation batch = rdbStore.initBatchOperation()) {
+
+ //given
+ String keyStr = RandomStringUtils.secure().next(10);
+ byte[] startKey = ("1-" + keyStr).getBytes(StandardCharsets.UTF_8);
+ byte[] keyInRange1 = ("2-" + keyStr).getBytes(StandardCharsets.UTF_8);
+ byte[] keyInRange2 = ("3-" + keyStr).getBytes(StandardCharsets.UTF_8);
+ byte[] endKey = ("4-" + keyStr).getBytes(StandardCharsets.UTF_8);
+ byte[] value =
+ RandomStringUtils.secure().next(10).getBytes(StandardCharsets.UTF_8);
+ testTable.put(startKey, value);
+ testTable.put(keyInRange1, value);
+ testTable.put(keyInRange2, value);
+ testTable.put(endKey, value);
+ assertNotNull(testTable.get(startKey));
+ assertNotNull(testTable.get(keyInRange1));
+ assertNotNull(testTable.get(keyInRange2));
+ assertNotNull(testTable.get(endKey));
+
+ //when
+ testTable.deleteRangeWithBatch(batch, startKey, endKey);
+ rdbStore.commitBatchOperation(batch);
+
+ //then
+ assertNull(testTable.get(startKey));
+ assertNull(testTable.get(keyInRange1));
+ assertNull(testTable.get(keyInRange2));
+ assertNotNull(testTable.get(endKey));
+ }
+ }
+
+ @Test
+ public void orderOfBatchOperations() throws Exception {
+ final Table<byte[], byte[]> testTable = rdbStore.getTable("Fifth");
+ try (BatchOperation batch = rdbStore.initBatchOperation()) {
+
+ //given
+ String keyStr = RandomStringUtils.secure().next(10);
+ byte[] startKey = ("1-" + keyStr).getBytes(StandardCharsets.UTF_8);
+ byte[] keyInRange1 = ("2-" + keyStr).getBytes(StandardCharsets.UTF_8);
+ byte[] endKey = ("3-" + keyStr).getBytes(StandardCharsets.UTF_8);
+ byte[] value1 = ("value1-" +
RandomStringUtils.secure().next(10)).getBytes(StandardCharsets.UTF_8);
+ byte[] value2 = ("value2-" +
RandomStringUtils.secure().next(10)).getBytes(StandardCharsets.UTF_8);
+ byte[] value3 = ("value3-" +
RandomStringUtils.secure().next(10)).getBytes(StandardCharsets.UTF_8);
+
+ //when
+ testTable.putWithBatch(batch, startKey, value1);
+ testTable.putWithBatch(batch, keyInRange1, value1);
+ testTable.deleteWithBatch(batch, keyInRange1);
+ // ops map key should be <<startKey, endKey>, 1>
+ testTable.deleteRangeWithBatch(batch, startKey, endKey);
+ testTable.putWithBatch(batch, startKey, value2);
+ testTable.putWithBatch(batch, keyInRange1, value2);
+ // ops map key is <<startKey, keyInRange1>, 2>.
+ testTable.deleteRangeWithBatch(batch, startKey, keyInRange1);
+ testTable.putWithBatch(batch, endKey, value1);
+ testTable.putWithBatch(batch, endKey, value2);
+ // ops map key is <<startKey, endKey>, 3>.
Review Comment:
The comments about the internal “ops map key” (e.g., `<<startKey, endKey>,
1>`) no longer match the current `RDBBatchOperation` implementation (ops are
keyed by an integer index and delete-ranges are separate ops). These comments
are misleading and should be updated or removed to avoid coupling the test to
stale internal details.
```suggestion
testTable.deleteRangeWithBatch(batch, startKey, endKey);
testTable.putWithBatch(batch, startKey, value2);
testTable.putWithBatch(batch, keyInRange1, value2);
testTable.deleteRangeWithBatch(batch, startKey, keyInRange1);
testTable.putWithBatch(batch, endKey, value1);
testTable.putWithBatch(batch, endKey, value2);
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java:
##########
@@ -332,6 +478,11 @@ void delete(CodecBuffer key) {
overwriteIfExists(new DeleteOp(key));
}
+ void deleteRange(byte[] startKey, byte[] endKey) {
Review Comment:
`FamilyCache.deleteRange(...)` bypasses the `isCommit` guard used by
put/delete (via `overwriteIfExists`). If `deleteRange` is called after
`prepareBatchWrite()` has started, the operation can be silently dropped (added
after `opList` is built). Add the same `Preconditions.checkState(!isCommit,
...)` protection here.
```suggestion
void deleteRange(byte[] startKey, byte[] endKey) {
Preconditions.checkState(!isCommit, "%s is already committed.",
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: [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]
