sumitagrawl commented on code in PR #7349:
URL: https://github.com/apache/ozone/pull/7349#discussion_r1822437886
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -257,8 +259,13 @@ public void start(OzoneConfiguration configuration) {
OZONE_BLOCK_DELETING_SERVICE_TIMEOUT,
OZONE_BLOCK_DELETING_SERVICE_TIMEOUT_DEFAULT,
TimeUnit.MILLISECONDS);
- dirDeletingService = new DirectoryDeletingService(dirDeleteInterval,
- TimeUnit.MILLISECONDS, serviceTimeout, ozoneManager, configuration);
+ int dirDeletingServiceCorePoolSize =
+ configuration.getInt(OZONE_THREAD_NUMBER_DIR_DELETION,
+ OZONE_THREAD_NUMBER_DIR_DELETION_DEFAULT);
+ dirDeletingService =
Review Comment:
can define min number of threads to avoid 0 or other value.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -135,17 +143,66 @@ public void resume() {
@Override
public BackgroundTaskQueue getTasks() {
BackgroundTaskQueue queue = new BackgroundTaskQueue();
- queue.add(new DirectoryDeletingService.DirDeletingTask(this));
+ BlockingQueue<Table.KeyValue<String, OmKeyInfo>> dirQueue =
+ new ArrayBlockingQueue<>(10000);
+ // Iterate the deleted dir table and push the dir omKeyInfo in an Array
list.
+ // Using a map to maintain unique elements in the list.
+ int count = 0;
+ final int MAX_COUNT = 10000;
+ try (
+ TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
deleteTableIterator = getOzoneManager().getMetadataManager()
Review Comment:
we can pass suplier giving next parent on need basis, this will avoid
unnescessary polling of records getting discarded if not consumed.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -135,17 +143,66 @@ public void resume() {
@Override
public BackgroundTaskQueue getTasks() {
BackgroundTaskQueue queue = new BackgroundTaskQueue();
- queue.add(new DirectoryDeletingService.DirDeletingTask(this));
+ BlockingQueue<Table.KeyValue<String, OmKeyInfo>> dirQueue =
+ new ArrayBlockingQueue<>(10000);
+ // Iterate the deleted dir table and push the dir omKeyInfo in an Array
list.
+ // Using a map to maintain unique elements in the list.
+ int count = 0;
+ final int MAX_COUNT = 10000;
+ try (
+ TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
deleteTableIterator = getOzoneManager().getMetadataManager()
+ .getDeletedDirTable().iterator()) {
+ Table.KeyValue<String, OmKeyInfo> deletedDirInfo;
+ while (count < MAX_COUNT && deleteTableIterator.hasNext()) {
+ deletedDirInfo = deleteTableIterator.next();
+ if (uniqueIDs.add(deletedDirInfo.getValue().getObjectID())) {
+ dirQueue.add(deletedDirInfo);
+ count++;
+ }
+ }
+ } catch (IOException e) {
+ LOG.error(
+ "Error while running delete directories and files " + "background
task. Will retry at next run.",
+ e);
+ }
+ int numThreads = dirQueue.size() < DIR_DELETING_CORE_POOL_SIZE ? 1 :
+ DIR_DELETING_CORE_POOL_SIZE;
+ for (int i = 0; i < numThreads; i++) {
+ queue.add(new DirectoryDeletingService.DirDeletingTask(this, dirQueue));
+ }
+ cleanUniqueIdSet(uniqueIDs);
return queue;
}
+ private void cleanUniqueIdSet(Set<Long> uniqueIDs) {
+ int count = 0;
+ if (uniqueIDs.size() > 40000) {
+ Iterator<Long> iterator = uniqueIDs.iterator();
+ while (iterator.hasNext()) {
+ iterator.next();
+ iterator.remove();
+ count++;
+ if (count >= 10000)
+ break;
+ }
+ }
+ }
+
private final class DirDeletingTask implements BackgroundTask {
private final DirectoryDeletingService directoryDeletingService;
+ BlockingQueue<Table.KeyValue<String, OmKeyInfo>> dirQueue =
Review Comment:
This must be initialized in constructor always as input
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -135,10 +142,65 @@ public void resume() {
@Override
public BackgroundTaskQueue getTasks() {
BackgroundTaskQueue queue = new BackgroundTaskQueue();
- queue.add(new DirectoryDeletingService.DirDeletingTask(this));
+ supplier.reInitItr();
+ for (int i = 0; i < DIR_DELETING_CORE_POOL_SIZE; i++) {
+ queue.add(new DirectoryDeletingService.DirDeletingTask(this));
+ }
return queue;
}
+ @Override
+ public void shutdown() {
+ super.shutdown();
+ supplier.closeItr();
+ }
+
+ private final class Supplier {
+ TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
+ deleteTableIterator;
+
+ private Supplier() throws IOException {
+ this.deleteTableIterator =
+ getOzoneManager().getMetadataManager().getDeletedDirTable()
+ .iterator();
+ }
+
+ private synchronized Table.KeyValue<String, OmKeyInfo> getItr() {
+ if (deleteTableIterator != null && !deleteTableIterator.hasNext()) {
+ try {
+ if (getOzoneManager().getMetadataManager().getDeletedDirTable()
Review Comment:
if iterator is closed, will next call for hasNext() have any issue? Can
simplify code, if table is not empty, reinit only.
if still it does not have next element, return null.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -135,10 +142,65 @@ public void resume() {
@Override
public BackgroundTaskQueue getTasks() {
BackgroundTaskQueue queue = new BackgroundTaskQueue();
- queue.add(new DirectoryDeletingService.DirDeletingTask(this));
+ supplier.reInitItr();
+ for (int i = 0; i < DIR_DELETING_CORE_POOL_SIZE; i++) {
+ queue.add(new DirectoryDeletingService.DirDeletingTask(this));
+ }
return queue;
}
+ @Override
+ public void shutdown() {
+ super.shutdown();
+ supplier.closeItr();
+ }
+
+ private final class Supplier {
Review Comment:
rename class to avoid conflict with java.util.function.Supplier,
DeletedDirSupplier
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -135,10 +142,65 @@ public void resume() {
@Override
public BackgroundTaskQueue getTasks() {
BackgroundTaskQueue queue = new BackgroundTaskQueue();
- queue.add(new DirectoryDeletingService.DirDeletingTask(this));
+ supplier.reInitItr();
+ for (int i = 0; i < DIR_DELETING_CORE_POOL_SIZE; i++) {
+ queue.add(new DirectoryDeletingService.DirDeletingTask(this));
+ }
return queue;
}
+ @Override
+ public void shutdown() {
+ super.shutdown();
+ supplier.closeItr();
+ }
+
+ private final class Supplier {
+ TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
+ deleteTableIterator;
+
+ private Supplier() throws IOException {
+ this.deleteTableIterator =
+ getOzoneManager().getMetadataManager().getDeletedDirTable()
+ .iterator();
+ }
+
+ private synchronized Table.KeyValue<String, OmKeyInfo> getItr() {
Review Comment:
Its getting element, naming should be get(), and it can return null value if
no further element. Caller should check for null.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -169,27 +231,24 @@ public BackgroundTaskResult call() {
= new ArrayList<>((int) remainNum);
Table.KeyValue<String, OmKeyInfo> pendingDeletedDirInfo;
-
- try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
- deleteTableIterator = getOzoneManager().getMetadataManager().
- getDeletedDirTable().iterator()) {
- // This is to avoid race condition b/w purge request and snapshot
chain updation. For AOS taking the global
- // snapshotId since AOS could process multiple buckets in one
iteration.
+ // This is to avoid race condition b/w purge request and snapshot
chain updation. For AOS taking the global
+ // snapshotId since AOS could process multiple buckets in one
iteration.
+ try {
UUID expectedPreviousSnapshotId =
-
((OmMetadataManagerImpl)getOzoneManager().getMetadataManager()).getSnapshotChainManager()
+ ((OmMetadataManagerImpl)
getOzoneManager().getMetadataManager()).getSnapshotChainManager()
.getLatestGlobalSnapshotId();
long startTime = Time.monotonicNow();
- while (remainNum > 0 && deleteTableIterator.hasNext()) {
- pendingDeletedDirInfo = deleteTableIterator.next();
+ while (remainNum > 0) {
+ pendingDeletedDirInfo = supplier.getItr();
Review Comment:
need to have null check , as supplier can return null if no futher element,
in this case, need exit loop.
--
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]
