Copilot commented on code in PR #9235:
URL: https://github.com/apache/ozone/pull/9235#discussion_r2511931116
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java:
##########
@@ -1254,6 +1260,7 @@ private static boolean assertTableRowCount(long
expectedCount,
LOG.info("{} actual row count={}, expectedCount={}", table.getName(),
count.get(), expectedCount);
});
+ System.out.println("Swaminathan \t" + count.get() + "\t" + expectedCount);
Review Comment:
Debug print statement should be removed before merging to production. Use
the existing LOG.info statement instead or remove entirely.
```suggestion
// Debug print statement removed; information is already logged above.
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1848,6 +1861,58 @@ public boolean containsIncompleteMPUs(String volume,
String bucket)
return false;
}
+ // NOTE: Update both getTableBucketPrefixInfo(volume, bucket) &
getTableBucketPrefix(tableName, volume, bucket)
+ // simultaneously. Implemented duplicate functions to avoid computing
bucketKeyPrefix redundantly for each and
+ // every table over and over again.
+ @Override
+ public TablePrefixInfo getTableBucketPrefix(String volume, String bucket)
throws IOException {
+ String keyPrefix = getBucketKeyPrefix(volume, bucket);
+ String keyPrefixFso = getBucketKeyPrefixFSO(volume, bucket);
+ // Set value to 12 to avoid creating too big a HashTable unnecessarily.
+ Map<String, String> tablePrefixMap = new HashMap<>(12, 1.0f);
+
+ tablePrefixMap.put(VOLUME_TABLE, getVolumeKey(volume));
+ tablePrefixMap.put(BUCKET_TABLE, getBucketKey(volume, bucket));
+
+ tablePrefixMap.put(KEY_TABLE, keyPrefix);
+ tablePrefixMap.put(DELETED_TABLE, keyPrefix);
+ tablePrefixMap.put(SNAPSHOT_RENAMED_TABLE, keyPrefix);
+ tablePrefixMap.put(OPEN_KEY_TABLE, keyPrefix);
+ tablePrefixMap.put(MULTIPART_INFO_TABLE, keyPrefix);
+ tablePrefixMap.put(SNAPSHOT_INFO_TABLE, keyPrefix);
+
+ tablePrefixMap.put(FILE_TABLE, keyPrefixFso);
+ tablePrefixMap.put(DIRECTORY_TABLE, keyPrefixFso);
+ tablePrefixMap.put(DELETED_DIR_TABLE, keyPrefixFso);
+ tablePrefixMap.put(OPEN_FILE_TABLE, keyPrefixFso);
+
+ return new TablePrefixInfo(tablePrefixMap);
+ }
+
+ @Override
+ public String getTableBucketPrefix(String tableName, String volume, String
bucket) throws IOException {
+ switch (tableName) {
+ case VOLUME_TABLE:
+ return getVolumeKey(volume);
+ case BUCKET_TABLE:
+ return getBucketKey(volume, bucket);
+ case KEY_TABLE:
+ case DELETED_TABLE:
+ case SNAPSHOT_RENAMED_TABLE:
+ case OPEN_KEY_TABLE:
+ case MULTIPART_INFO_TABLE:
+ case SNAPSHOT_INFO_TABLE:
+ return getBucketKeyPrefix(volume, bucket);
+ case FILE_TABLE:
+ case DIRECTORY_TABLE:
+ case DELETED_DIR_TABLE:
+ case OPEN_FILE_TABLE:
+ return getBucketKeyPrefixFSO(volume, bucket);
+ default:
Review Comment:
Returning an empty string as default for unrecognized table names could
silently mask configuration errors. Consider logging a warning when an unknown
table name is encountered to aid debugging.
```suggestion
default:
LOG.warn("Unknown table name '{}' passed to getTableBucketPrefix
(volume='{}', bucket='{}'). Returning empty string.", tableName, volume,
bucket);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1848,6 +1861,58 @@ public boolean containsIncompleteMPUs(String volume,
String bucket)
return false;
}
+ // NOTE: Update both getTableBucketPrefixInfo(volume, bucket) &
getTableBucketPrefix(tableName, volume, bucket)
Review Comment:
The comment references 'getTableBucketPrefixInfo' but the actual method name
is 'getTableBucketPrefix' (without 'Info'). Update the comment to match the
actual method signature.
```suggestion
// NOTE: Update both getTableBucketPrefix(volume, bucket) &
getTableBucketPrefix(tableName, volume, bucket)
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/StringUtils.java:
##########
@@ -123,4 +123,11 @@ public static String
getLexicographicallyHigherString(String val) {
charVal[lastIdx] += 1;
return String.valueOf(charVal);
}
+
+ public static String getFirstNChars(String str, int n) {
+ if (str == null || str.length() < n) {
+ return str;
+ }
+ return str.substring(0, n);
+ }
Review Comment:
The method returns the original string when `str.length() < n`, but should
return the full string only when it's shorter. When `str` is null, returning
null may cause NullPointerExceptions in callers. Consider returning an empty
string for null input or documenting this behavior clearly.
--
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]
