Copilot commented on code in PR #8964:
URL: https://github.com/apache/ozone/pull/8964#discussion_r2299321458
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -526,31 +527,51 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
return dbCheckpoint;
}
+ /**
+ * Helper method to perform batch delete range operation on a given key
prefix.
+ * @param prefix prefix of keys to be deleted
+ * @param table table from which keys are to be deleted
+ * @param batchOperation batch operation
+ */
+ private static void deleteKeysFromTableWithPrefix(
+ String prefix, Table<String, ?> table, BatchOperation batchOperation)
throws IOException {
+ String endKey = getLexicographicallyHigherString(prefix);
+ LOG.debug("Deleting key range from {} - startKey: {}, endKey: {}",
+ table.getName(), prefix, endKey);
+ table.deleteRangeWithBatch(batchOperation, prefix, endKey);
+ }
+
/**
* Helper method to delete DB keys in the snapshot scope (bucket)
* from active DB's deletedDirectoryTable.
* @param omMetadataManager OMMetadataManager instance
* @param volumeName volume name
* @param bucketName bucket name
+ * @param batchOperation batch operation
+ */
+ private static void deleteKeysFromSnapRenamedTableInSnapshotScope(
+ OMMetadataManager omMetadataManager, String volumeName,
+ String bucketName, BatchOperation batchOperation) throws IOException {
+
+ final String keyPrefix = omMetadataManager.getBucketKeyPrefix(volumeName,
bucketName);
+ deleteKeysFromTableWithPrefix(keyPrefix,
omMetadataManager.getSnapshotRenamedTable(), batchOperation);
+ }
+
+ /**
+ * Helper method to delete DB keys in the snapshot scope (bucket)
+ * from active DB's deletedDirectoryTable.
+ * @param omMetadataManager OMMetadataManager instance
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @param batchOperation batch operation
*/
private static void deleteKeysFromDelDirTableInSnapshotScope(
OMMetadataManager omMetadataManager, String volumeName,
String bucketName, BatchOperation batchOperation) throws IOException {
// Range delete start key (inclusive)
- final String keyPrefix =
omMetadataManager.getBucketKeyPrefixFSO(volumeName, bucketName);
-
- try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
- iter = omMetadataManager.getDeletedDirTable().iterator(keyPrefix)) {
- performOperationOnKeys(iter,
- entry -> {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Removing key {} from DeletedDirTable",
entry.getKey());
- }
-
omMetadataManager.getDeletedDirTable().deleteWithBatch(batchOperation,
entry.getKey());
- return null;
- });
- }
+ final String keyPreix =
omMetadataManager.getBucketKeyPrefixFSO(volumeName, bucketName);
+ deleteKeysFromTableWithPrefix(keyPreix,
omMetadataManager.getDeletedDirTable(), batchOperation);
Review Comment:
There's a typo in the variable name 'keyPreix' - it should be 'keyPrefix'.
```suggestion
final String keyPrefix =
omMetadataManager.getBucketKeyPrefixFSO(volumeName, bucketName);
deleteKeysFromTableWithPrefix(keyPrefix,
omMetadataManager.getDeletedDirTable(), batchOperation);
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotCreateRequest.java:
##########
@@ -364,10 +384,103 @@ public void testSnapshotLimitWithFailures() throws
Exception {
assertEquals(OMException.ResultCodes.TOO_MANY_SNAPSHOTS,
omException.getResult());
}
- private void renameKey(String fromKey, String toKey, long offset)
+ @Test
+ public void testEntryDeletedTable() throws Exception {
+ when(getOzoneManager().isAdmin(any())).thenReturn(true);
+ Table<String, RepeatedOmKeyInfo> deletedTable =
getOmMetadataManager().getDeletedTable();
+
+ // 1. Create a second bucket with lexicographically higher name
+ String bucket1Name = getBucketName();
+ String bucket2Name = getBucketName() + "0";
+ String volumeName = getVolumeName();
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucket2Name,
getOmMetadataManager());
+
+ // 2. Add and delete keys from both buckets
+ OmKeyInfo key1 = addKeyInBucket(volumeName, bucket1Name, "key1", 100L);
+ OmKeyInfo key2 = addKeyInBucket(volumeName, bucket2Name, "key2", 200L);
+ deleteKey(key1);
+ deleteKey(key2);
+
+ // 3. Verify deletedTable contains both deleted keys (2 rows)
+ assertEquals(2, getOmMetadataManager().countRowsInTable(deletedTable));
+
+ // 4. Create a snapshot on bucket1
+ createSnapshot(snapshotName1);
+
+ // 5. Verify deletedTable now only contains the key from bucket2 (1 row)
+ assertEquals(1, getOmMetadataManager().countRowsInTable(deletedTable));
+ // Verify the remaining entry is from bucket2
+ try (TableIterator<String, ? extends Table.KeyValue<String,
RepeatedOmKeyInfo>> iter = deletedTable.iterator()) {
+ iter.seekToFirst();
+ while (iter.hasNext()) {
+ String key = iter.next().getKey();
+
assertTrue(key.startsWith(getOmMetadataManager().getBucketKeyPrefix(volumeName,
bucket2Name)),
+ "Key should be from bucket2: " + key);
+ }
+ }
+
+
+ }
+
+ @Test
+ public void testEntryDeletedDirTable() throws Exception {
+ when(getOzoneManager().isAdmin(any())).thenReturn(true);
+ Table<String, OmKeyInfo> deletedDirTable =
getOmMetadataManager().getDeletedDirTable();
+
+ // 1. Create a second bucket with lexicographically higher name
+ String bucket1Name = getBucketName();
+ String bucket2Name = getBucketName() + "0";
+ String volumeName = getVolumeName();
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucket2Name,
getOmMetadataManager());
+
+ // 2. Add and delete keys from both buckets
+ OmKeyInfo key1 = addKeyInBucket(volumeName, bucket1Name, "dir2", 100L);
+ OmKeyInfo key2 = addKeyInBucket(volumeName, bucket2Name, "dir20", 200L);
+ deleteDirectory(key1);
+ deleteDirectory(key2);
+
+ // 3. Verify deletedDirTable contains both deleted keys (2 rows)
+ assertEquals(2, getOmMetadataManager().countRowsInTable(deletedDirTable));
+
+ // 4. Create a snapshot on bucket1
+ createSnapshotForBucket(volumeName, bucket1Name, snapshotName1);
+
+ // 5. Verify deletedTable now only contains the key from bucket2 (1 row)
+ assertEquals(1, getOmMetadataManager().countRowsInTable(deletedDirTable));
+ // Verify the remaining entry is from bucket2
+ try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
iter = deletedDirTable.iterator()) {
+ while (iter.hasNext()) {
+ String key = iter.next().getKey();
+
assertTrue(key.startsWith(getOmMetadataManager().getBucketKeyPrefixFSO(volumeName,
bucket2Name)),
+ "Key should be from bucket2: " + key);
+ }
+ }
+ }
+
+ private void deleteDirectory(OmKeyInfo dirInfo) throws IOException {
+ String dirKey = getOmMetadataManager().getOzonePathKey(
+ getOmMetadataManager().getVolumeId(dirInfo.getVolumeName()),
+ getOmMetadataManager().getBucketId(dirInfo.getVolumeName(),
dirInfo.getBucketName()),
+ dirInfo.getParentObjectID(), dirInfo.getKeyName());
+ System.out.println("Deleting directory: " + dirKey);
Review Comment:
Remove debug print statement or replace with proper logging using
LOG.debug().
```suggestion
LOG.debug("Deleting directory: {}", dirKey);
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/StringUtils.java:
##########
@@ -97,4 +97,16 @@ public static String bytes2String(byte[] bytes) {
public static byte[] string2Bytes(String str) {
return str.getBytes(UTF8);
}
+
+ public static String getLexicographicallyLowerString(String val) {
+ char[] charVal = val.toCharArray();
+ charVal[charVal.length - 1] -= 1;
+ return String.valueOf(charVal);
+ }
+
+ public static String getLexicographicallyHigherString(String val) {
+ char[] charVal = val.toCharArray();
+ charVal[charVal.length - 1] += 1;
Review Comment:
These methods lack input validation for null or empty strings and could
cause IndexOutOfBoundsException. Consider adding null/empty checks and handling
edge cases like character overflow.
```suggestion
if (val == null || val.isEmpty()) {
throw new IllegalArgumentException("Input string must not be null or
empty");
}
char[] charVal = val.toCharArray();
int lastIdx = charVal.length - 1;
if (charVal[lastIdx] == Character.MIN_VALUE) {
throw new IllegalArgumentException("Cannot decrement character below
Character.MIN_VALUE");
}
charVal[lastIdx] -= 1;
return String.valueOf(charVal);
}
public static String getLexicographicallyHigherString(String val) {
if (val == null || val.isEmpty()) {
throw new IllegalArgumentException("Input string must not be null or
empty");
}
char[] charVal = val.toCharArray();
int lastIdx = charVal.length - 1;
if (charVal[lastIdx] == Character.MAX_VALUE) {
throw new IllegalArgumentException("Cannot increment character above
Character.MAX_VALUE");
}
charVal[lastIdx] += 1;
```
--
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]
