swamirishi commented on code in PR #8964:
URL: https://github.com/apache/ozone/pull/8964#discussion_r2295697988
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -532,25 +534,52 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
* @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 startKey = omMetadataManager.getBucketKey(volumeName,
bucketName) + OM_KEY_PREFIX;
+ // endKey is the smallest key that is lexicographically larger than the
startKey. (exclusive)
+ final String endKey = startKey.substring(0, startKey.length() - 1) +
Review Comment:
Can we move this function to Util class and use it here
https://github.com/apache/ozone/blob/83fd8d7a31ca3e0b0c98a4dc7beb3c4e5aa5769d/hadoop-hdds/rocks-native/src/test/java/org/apache/hadoop/hdds/utils/TestUtils.java#L36-L46
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -507,12 +505,16 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
OmSnapshotManager.createNewOmSnapshotLocalDataFile(omMetadataManager,
snapshotInfo, store);
// Clean up active DB's deletedTable right after checkpoint is taken,
- // There is no need to take any lock as of now, because transactions are
flushed sequentially.
+ // Snapshot create is processed as a single transactions and
Review Comment:
nit:
```suggestion
// Snapshot create is processed as a single transaction and
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -532,25 +534,52 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
* @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 startKey = omMetadataManager.getBucketKey(volumeName,
bucketName) + OM_KEY_PREFIX;
+ // endKey is the smallest key that is lexicographically larger than the
startKey. (exclusive)
+ final String endKey = startKey.substring(0, startKey.length() - 1) +
+ (char)(startKey.charAt(startKey.length() - 1) + 1);
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Deleting key range from SnapshotRenamedTable - startKey: {},
endKey: {}",
+ startKey, endKey);
+ }
+
+ // Remove all entries from snapshotRenamedTable using deleteRange API
+
omMetadataManager.getSnapshotRenamedTable().deleteRangeWithBatch(batchOperation,
startKey, 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 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 startKey =
omMetadataManager.getBucketKeyPrefixFSO(volumeName, bucketName);
+
+ // endKey is the smallest key that is lexicographically larger than the
startKey. (exclusive)
+ final String endKey = startKey.substring(0, startKey.length() - 1) +
+ (char)(startKey.charAt(startKey.length() - 1) + 1);
+
+ if (LOG.isDebugEnabled()) {
Review Comment:
I see the same repeated logic everywhere. Can we create a function:
deleteKeysFromTableWithPrefix(String prefix, Table<String, ?> table) {
String endKey = getLexicographicallyHigherString(prefix);
LOG.debug("Deleting key range from DeletedDirTable - startKey: {},
endKey: {}",
prefix, endKey);
table.deleteRangeWithBatch(batchOperation, prefix, endKey);
}
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -563,64 +592,33 @@ public SnapshotDiffCleanupService
getSnapshotDiffCleanupService() {
return snapshotDiffCleanupService;
}
- /**
- * Helper method to perform operation on keys with a given iterator.
- * @param keyIter TableIterator
- * @param operationFunction operation to be performed for each key.
- */
- private static void performOperationOnKeys(
- TableIterator<String, ? extends Table.KeyValue<String, ?>> keyIter,
- CheckedFunction<Table.KeyValue<String, ?>,
- Void, IOException> operationFunction) throws IOException {
- // Continue only when there are entries of snapshot (bucket) scope
- // in deletedTable in the first place
- // Loop until prefix matches.
- // Start performance tracking timer
- long startTime = System.nanoTime();
- while (keyIter.hasNext()) {
- Table.KeyValue<String, ?> entry = keyIter.next();
- operationFunction.apply(entry);
- }
- // Time took for the iterator to finish (in ns)
- long timeElapsed = System.nanoTime() - startTime;
- if (timeElapsed >= DB_TABLE_ITER_LOOP_THRESHOLD_NS) {
- // Print time elapsed
- LOG.warn("Took {} ns to find endKey. Caller is {}", timeElapsed,
- new Throwable().fillInStackTrace().getStackTrace()[1]
- .getMethodName());
- }
- }
-
/**
* Helper method to delete DB keys in the snapshot scope (bucket)
* from active DB's deletedTable.
* @param omMetadataManager OMMetadataManager instance
* @param volumeName volume name
* @param bucketName bucket name
+ * @param batchOperation batch operation
*/
private static void deleteKeysFromDelKeyTableInSnapshotScope(
OMMetadataManager omMetadataManager, String volumeName,
String bucketName, BatchOperation batchOperation) throws IOException {
// Range delete start key (inclusive)
- final String keyPrefix =
- omMetadataManager.getBucketKeyPrefix(volumeName, bucketName);
-
- try (TableIterator<String,
- ? extends Table.KeyValue<String, RepeatedOmKeyInfo>>
- iter = omMetadataManager.getDeletedTable().iterator(keyPrefix)) {
- performOperationOnKeys(iter, entry -> {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Removing key {} from DeletedTable", entry.getKey());
- }
- omMetadataManager.getDeletedTable().deleteWithBatch(batchOperation,
entry.getKey());
- return null;
- });
+ final String startKey = omMetadataManager.getBucketKeyPrefix(volumeName,
bucketName);
+ // endKey is the smallest key that is lexicographically larger than the
startKey. (exclusive)
+ final String endKey = startKey.substring(0, startKey.length() - 1) +
+ (char)(startKey.charAt(startKey.length() - 1) + 1);
+
+ if (LOG.isDebugEnabled()) {
Review Comment:
No need for this if statement
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -532,25 +534,52 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
* @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 startKey = omMetadataManager.getBucketKey(volumeName,
bucketName) + OM_KEY_PREFIX;
Review Comment:
Please use OmMetadataManager.getBucketKeyPrefix() instead
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -532,25 +534,52 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
* @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 startKey = omMetadataManager.getBucketKey(volumeName,
bucketName) + OM_KEY_PREFIX;
+ // endKey is the smallest key that is lexicographically larger than the
startKey. (exclusive)
+ final String endKey = startKey.substring(0, startKey.length() - 1) +
+ (char)(startKey.charAt(startKey.length() - 1) + 1);
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Deleting key range from SnapshotRenamedTable - startKey: {},
endKey: {}",
+ startKey, endKey);
+ }
+
+ // Remove all entries from snapshotRenamedTable using deleteRange API
+
omMetadataManager.getSnapshotRenamedTable().deleteRangeWithBatch(batchOperation,
startKey, 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 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 startKey =
omMetadataManager.getBucketKeyPrefixFSO(volumeName, bucketName);
+
+ // endKey is the smallest key that is lexicographically larger than the
startKey. (exclusive)
+ final String endKey = startKey.substring(0, startKey.length() - 1) +
Review Comment:
Use util function
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -532,25 +534,52 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
* @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 startKey = omMetadataManager.getBucketKey(volumeName,
bucketName) + OM_KEY_PREFIX;
+ // endKey is the smallest key that is lexicographically larger than the
startKey. (exclusive)
+ final String endKey = startKey.substring(0, startKey.length() - 1) +
+ (char)(startKey.charAt(startKey.length() - 1) + 1);
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Deleting key range from SnapshotRenamedTable - startKey: {},
endKey: {}",
+ startKey, endKey);
+ }
+
+ // Remove all entries from snapshotRenamedTable using deleteRange API
+
omMetadataManager.getSnapshotRenamedTable().deleteRangeWithBatch(batchOperation,
startKey, 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 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 startKey =
omMetadataManager.getBucketKeyPrefixFSO(volumeName, bucketName);
+
+ // endKey is the smallest key that is lexicographically larger than the
startKey. (exclusive)
+ final String endKey = startKey.substring(0, startKey.length() - 1) +
+ (char)(startKey.charAt(startKey.length() - 1) + 1);
+
+ if (LOG.isDebugEnabled()) {
Review Comment:
No need for if condition
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -563,64 +592,33 @@ public SnapshotDiffCleanupService
getSnapshotDiffCleanupService() {
return snapshotDiffCleanupService;
}
- /**
- * Helper method to perform operation on keys with a given iterator.
- * @param keyIter TableIterator
- * @param operationFunction operation to be performed for each key.
- */
- private static void performOperationOnKeys(
- TableIterator<String, ? extends Table.KeyValue<String, ?>> keyIter,
- CheckedFunction<Table.KeyValue<String, ?>,
- Void, IOException> operationFunction) throws IOException {
- // Continue only when there are entries of snapshot (bucket) scope
- // in deletedTable in the first place
- // Loop until prefix matches.
- // Start performance tracking timer
- long startTime = System.nanoTime();
- while (keyIter.hasNext()) {
- Table.KeyValue<String, ?> entry = keyIter.next();
- operationFunction.apply(entry);
- }
- // Time took for the iterator to finish (in ns)
- long timeElapsed = System.nanoTime() - startTime;
- if (timeElapsed >= DB_TABLE_ITER_LOOP_THRESHOLD_NS) {
- // Print time elapsed
- LOG.warn("Took {} ns to find endKey. Caller is {}", timeElapsed,
- new Throwable().fillInStackTrace().getStackTrace()[1]
- .getMethodName());
- }
- }
-
/**
* Helper method to delete DB keys in the snapshot scope (bucket)
* from active DB's deletedTable.
* @param omMetadataManager OMMetadataManager instance
* @param volumeName volume name
* @param bucketName bucket name
+ * @param batchOperation batch operation
*/
private static void deleteKeysFromDelKeyTableInSnapshotScope(
OMMetadataManager omMetadataManager, String volumeName,
String bucketName, BatchOperation batchOperation) throws IOException {
// Range delete start key (inclusive)
- final String keyPrefix =
- omMetadataManager.getBucketKeyPrefix(volumeName, bucketName);
-
- try (TableIterator<String,
- ? extends Table.KeyValue<String, RepeatedOmKeyInfo>>
- iter = omMetadataManager.getDeletedTable().iterator(keyPrefix)) {
- performOperationOnKeys(iter, entry -> {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Removing key {} from DeletedTable", entry.getKey());
- }
- omMetadataManager.getDeletedTable().deleteWithBatch(batchOperation,
entry.getKey());
- return null;
- });
+ final String startKey = omMetadataManager.getBucketKeyPrefix(volumeName,
bucketName);
+ // endKey is the smallest key that is lexicographically larger than the
startKey. (exclusive)
+ final String endKey = startKey.substring(0, startKey.length() - 1) +
Review Comment:
Use this function
https://github.com/apache/ozone/blob/83fd8d7a31ca3e0b0c98a4dc7beb3c4e5aa5769d/hadoop-hdds/rocks-native/src/test/java/org/apache/hadoop/hdds/utils/TestUtils.java#L36-L46
--
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]
