You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/10/20 14:44:04 UTC
[GitHub] [hadoop-ozone] sadanand48 opened a new pull request #1507: HDDS-4307.Start Background Service for Trash Deletion in Ozone Manager
sadanand48 opened a new pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507
## What changes were proposed in this pull request?
The change here is to implement a background service for Trash Deletion. This feature basically introduces Trash for Ozone File System by leveraging the hadoop code for Trash ( `Trash.java`) . When a client does a delete on the filesystem, it calls the `fs.rename( src,dst,Options)` api which does a rename to the Trash Path. As an initial approach all operations are on the `KeyTable` itself.
This pr starts a Background Service similar to KeyDeletingService/BlockDeletingService in Ozone which in turn calls the TrashEmptier and deletes all the files obtained by` fs.getTrashRoots()`. By passing an OFS uri pointing to the root to this `fs ` instance covering all volumes and buckets will be ensured. Thus the background service starts the emptier thread which basically iterates through the trash roots and does the checkpointing and then deletion from checkpointed directories.
The idea behind starting a background service here is to extend it so that multiple threads can perform the Trash Deletion. Currently with respect to this pr, only a single thread is used.
**bin/ozone fs -rm o3fs://buck1.vol1/Test2**
`2020-10-20 18:42:12,779 [main] INFO fs.TrashPolicyDefault: Moved: 'o3fs://buck1.vol1/Test2' to trash at: /.Trash/sadanand.shenoy/Current/Test2`
**bin/ozone fs -ls -R o3fs://buck1.vol1/**
`-rw-rw-rw- 1 sadanand.shenoy sadanand.shenoy 0 2020-10-20 18:42 o3fs://buck1.vol1/.Trash/sadanand.shenoy/Current/Test2`
**after checkpoint interval**
`-rw-rw-rw- 1 sadanand.shenoy sadanand.shenoy 0 2020-10-20 18:45 o3fs://buck1.vol1/.Trash/sadanand.shenoy/201020184500/Test2`
The key gets deleted after the trash interval.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4307
## How was this patch tested?
Manually as well as few unit Tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sadanand48 edited a comment on pull request #1507: HDDS-4307.Start Trash Emptier in Ozone Manager
Posted by GitBox <gi...@apache.org>.
sadanand48 edited a comment on pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#issuecomment-717002589
Thanks @rakeshadr for the review. Addressed your comments . Will incorporate the changes you suggested on the TrashDeletingService when i include it in the next patch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sadanand48 commented on a change in pull request #1507: HDDS-4307.Start Trash Emptier in Ozone Manager
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on a change in pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#discussion_r512567525
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1228,17 +1238,57 @@ public void restart() throws IOException {
// Allow OM to start as Http Server failure is not fatal.
LOG.error("OM HttpServer failed to start.", ex);
}
-
omRpcServer.start();
+
isOmRpcServerRunning = true;
+ startTrashEmptier(configuration);
+
registerMXBean();
startJVMPauseMonitor();
setStartTime();
omState = State.RUNNING;
}
+
+ /**
+ * @param conf
+ * @throws IOException
+ * Starts a Trash Emptier thread that does an fs.trashRoots and performs
+ * checkpointing & deletion
+ */
+ private void startTrashEmptier(Configuration conf) throws IOException {
+ long trashInterval =
+ conf.getLong(FS_TRASH_INTERVAL_KEY, FS_TRASH_INTERVAL_DEFAULT);
+ if (trashInterval == 0) {
+ return;
Review comment:
Done.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1228,17 +1238,57 @@ public void restart() throws IOException {
// Allow OM to start as Http Server failure is not fatal.
LOG.error("OM HttpServer failed to start.", ex);
}
-
omRpcServer.start();
+
isOmRpcServerRunning = true;
+ startTrashEmptier(configuration);
+
registerMXBean();
startJVMPauseMonitor();
setStartTime();
omState = State.RUNNING;
}
+
+ /**
+ * @param conf
+ * @throws IOException
+ * Starts a Trash Emptier thread that does an fs.trashRoots and performs
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] mukul1987 commented on a change in pull request #1507: HDDS-4307.Start Background Service for Trash Deletion in Ozone Manager
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#discussion_r508628274
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashDeletingService.java
##########
@@ -0,0 +1,114 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Trash;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.security.SecurityUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
+
+/**
+ * Background Service to empty keys that are moved to Trash.
+ */
+public class TrashDeletingService extends BackgroundService {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TrashDeletingService.class);
+
+ // Use single thread for now
+ private final static int KEY_DELETING_CORE_POOL_SIZE = 1;
+
+ private OzoneManager ozoneManager;
+
+ public void setFsConf(Configuration fsConf) {
+ this.fsConf = fsConf;
Review comment:
The indentation is at 4, it should be at 2.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashDeletingService.java
##########
@@ -0,0 +1,114 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Trash;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.security.SecurityUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
+
+/**
+ * Background Service to empty keys that are moved to Trash.
+ */
+public class TrashDeletingService extends BackgroundService {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TrashDeletingService.class);
+
+ // Use single thread for now
+ private final static int KEY_DELETING_CORE_POOL_SIZE = 1;
+
+ private OzoneManager ozoneManager;
+
+ public void setFsConf(Configuration fsConf) {
+ this.fsConf = fsConf;
+ }
+
+ private Configuration fsConf;
+
+ public TrashDeletingService(long interval, long serviceTimeout,OzoneManager ozoneManager) {
+ super("TrashDeletingService", interval, TimeUnit.MILLISECONDS, KEY_DELETING_CORE_POOL_SIZE, serviceTimeout);
+ this.ozoneManager = ozoneManager;
+ fsConf = new Configuration();
+ }
+
+
+ @Override
+ public BackgroundTaskQueue getTasks() {
+ BackgroundTaskQueue queue = new BackgroundTaskQueue();
+ String rootPath = String.format("%s://%s/",
+ OzoneConsts.OZONE_OFS_URI_SCHEME, ozoneManager.getConfiguration().get(OZONE_OM_ADDRESS_KEY));
+ // Configuration object where the rootpath is set to an OFS Uri.
+ fsConf.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath);
+ FileSystem fs = null;
+ try {
+ fs = SecurityUtil.doAsLoginUser(
+ new PrivilegedExceptionAction<FileSystem>() {
+ @Override
+ public FileSystem run() throws IOException {
+ return FileSystem.get(fsConf);
+ }
+ });
+ } catch (IOException e) {
+ LOG.error("Cannot instantiate filesystem instance");
+ }
+ queue.add(new TrashDeletingTask(fs,fsConf));
+ return queue;
+ }
+
+ /**
+ * This task creates an emptier thread that deletes all keys obtained from the trashRoots (fs.getTrashRoots)
+ */
+ private class TrashDeletingTask implements BackgroundTask {
+
+ FileSystem fs;
+ Configuration conf;
+
+ public TrashDeletingTask(FileSystem fs, Configuration conf) {
+ this.fs = fs;
+ this.conf = conf;
+ }
+
+ @Override
+ public BackgroundTaskResult call() throws Exception {
+ Thread emptier = new Thread(new Trash(fs,conf).getEmptier(),"Trash Emptier");
Review comment:
This will start 1 thread for the entire filesystem for every run? why do we need this a background service ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] rakeshadr commented on a change in pull request #1507: HDDS-4307.Start Background Service for Trash Deletion in Ozone Manager
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#discussion_r509870311
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1228,17 +1236,34 @@ public void restart() throws IOException {
// Allow OM to start as Http Server failure is not fatal.
LOG.error("OM HttpServer failed to start.", ex);
}
-
omRpcServer.start();
+
isOmRpcServerRunning = true;
+ startTrashDeletingService();
+
registerMXBean();
startJVMPauseMonitor();
setStartTime();
omState = State.RUNNING;
}
+ private void startTrashDeletingService() {
+ if (trashDeletingService == null) {
+ long serviceTimeout = configuration.getTimeDuration(
+ OZONE_TRASH_DELETING_SERVICE_TIMEOUT,
+ OZONE_TRASH_DELETING_SERVICE_TIMEOUT_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ long trashDeletionInterval = configuration.getTimeDuration(
+ OZONE_TRASH_DELETING_SERVICE_INTERVAL,
+ OZONE_TRASH_DELETING_SERVICE_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ trashDeletingService = new TrashDeletingService(trashDeletionInterval, serviceTimeout, this);
+ trashDeletingService.start();
Review comment:
Please shutdown the `trashDeletingService` during OM stop.
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -1170,4 +1172,50 @@ public void testFileDelete() throws Exception {
Boolean falseResult = fs.delete(parent, true);
assertFalse(falseResult);
}
+
+ /**
+ * @throws Exception
+ * 1.Move a Key to Trash
+ * 2.Start TrashDeletingService
+ * 3.Verify that the TrashDeletingService purges the key after minimum set TrashInterval of 1 min.
+ */
+ @Test
+ public void testTrashDeletingService() throws Exception {
+ String testKeyName = "keyToBeDeleted";
+ Path path = new Path(bucketPath, testKeyName);
+ try (FSDataOutputStream stream = fs.create(path)) {
+ stream.write(1);
+ }
+ // Call moveToTrash. We can't call protected fs.rename() directly
+ trash.moveToTrash(path);
+ TrashDeletingService trashDeletingService = new
+ TrashDeletingService(60,300,cluster.getOzoneManager());
+ conf.setLong(FS_TRASH_INTERVAL_KEY,1);
+ trashDeletingService.setFsConf(conf);
+ trashDeletingService.start();
+
+
+ // Construct paths
+ String username = UserGroupInformation.getCurrentUser().getShortUserName();
+ Path trashRoot = new Path(bucketPath, TRASH_PREFIX);
+ Path userTrash = new Path(trashRoot, username);
+ Path userTrashCurrent = new Path(userTrash, "Current");
+ String key = path.toString().substring(1);
+ Path trashPath = new Path(userTrashCurrent, key);
+
+ // Wait until the TrashDeletingService purges the key
+ GenericTestUtils.waitFor(()-> {
+ try {
+ return !ofs.exists(trashPath);
+ } catch (IOException e) {
+ LOG.error("Delete from Trash Failed");
+ Assert.fail();
Review comment:
Please add failure message -> Assert.fail("Delete from Trash Failed");
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashDeletingService.java
##########
@@ -0,0 +1,114 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Trash;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.security.SecurityUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
+
+/**
+ * Background Service to empty keys that are moved to Trash.
+ */
+public class TrashDeletingService extends BackgroundService {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TrashDeletingService.class);
+
+ // Use single thread for now
+ private final static int KEY_DELETING_CORE_POOL_SIZE = 1;
+
+ private OzoneManager ozoneManager;
+
+ public void setFsConf(Configuration fsConf) {
+ this.fsConf = fsConf;
+ }
+
+ private Configuration fsConf;
+
+ public TrashDeletingService(long interval, long serviceTimeout,OzoneManager ozoneManager) {
+ super("TrashDeletingService", interval, TimeUnit.MILLISECONDS, KEY_DELETING_CORE_POOL_SIZE, serviceTimeout);
+ this.ozoneManager = ozoneManager;
+ fsConf = new Configuration();
Review comment:
Please do `ozoneManager.getConfiguration()` instead of loading `new Configuration()` here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] rakeshadr commented on pull request #1507: HDDS-4307.Start Background Service for Trash Deletion in Ozone Manager
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#issuecomment-714260856
Thanks @sadanand48 for the contribution. I've added few comments, please go through it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] rakeshadr commented on a change in pull request #1507: HDDS-4307.Start Background Service for Trash Deletion in Ozone Manager
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#discussion_r509870897
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashDeletingService.java
##########
@@ -0,0 +1,114 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Trash;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.security.SecurityUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
+
+/**
+ * Background Service to empty keys that are moved to Trash.
+ */
+public class TrashDeletingService extends BackgroundService {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TrashDeletingService.class);
+
+ // Use single thread for now
+ private final static int KEY_DELETING_CORE_POOL_SIZE = 1;
+
+ private OzoneManager ozoneManager;
+
+ public void setFsConf(Configuration fsConf) {
+ this.fsConf = fsConf;
+ }
+
+ private Configuration fsConf;
+
+ public TrashDeletingService(long interval, long serviceTimeout,OzoneManager ozoneManager) {
+ super("TrashDeletingService", interval, TimeUnit.MILLISECONDS, KEY_DELETING_CORE_POOL_SIZE, serviceTimeout);
+ this.ozoneManager = ozoneManager;
+ fsConf = new Configuration();
Review comment:
I saw you are setting `fsConf.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath);` to get FS. Is that the reason for creating `new Configuration()` instead of using `ozoneManager.getConfiguration()` ?
If yes, can you please add comments mentioning the reason.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sadanand48 commented on pull request #1507: HDDS-4307.Start Trash Emptier in Ozone Manager
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#issuecomment-717002589
Thanks @rakeshadr for the review. Addressed your comments . Will incorporate the changes you suggested on the BackgroundService when i include it in the next patch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] prashantpogde commented on pull request #1507: HDDS-4307.Start Background Service for Trash Deletion in Ozone Manager
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#issuecomment-714205605
Can you address the CI failures ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] prashantpogde commented on pull request #1507: HDDS-4307.Start Trash Emptier in Ozone Manager
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#issuecomment-717415113
+1 LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] rakeshadr commented on a change in pull request #1507: HDDS-4307.Start Trash Emptier in Ozone Manager
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#discussion_r512525048
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1228,17 +1238,57 @@ public void restart() throws IOException {
// Allow OM to start as Http Server failure is not fatal.
LOG.error("OM HttpServer failed to start.", ex);
}
-
omRpcServer.start();
+
isOmRpcServerRunning = true;
+ startTrashEmptier(configuration);
+
registerMXBean();
startJVMPauseMonitor();
setStartTime();
omState = State.RUNNING;
}
+
+ /**
+ * @param conf
+ * @throws IOException
+ * Starts a Trash Emptier thread that does an fs.trashRoots and performs
Review comment:
Please follow general guidelines for the javadoc.
1) Begins with function details.
2) Provide `@param` details.
3) Then `@return` info.
4) Ending with `@throws` exception cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] rakeshadr commented on a change in pull request #1507: HDDS-4307.Start Background Service for Trash Deletion in Ozone Manager
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#discussion_r509891414
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -197,6 +197,12 @@
public static final String OZONE_BLOCK_DELETING_SERVICE_INTERVAL_DEFAULT
= "60s";
+ public static final String OZONE_TRASH_DELETING_SERVICE_INTERVAL =
Review comment:
Please add the new configs to `ozone-default.xm`l. Thats the reason for `TestOzoneConfigurationFields` unit test failure.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashDeletingService.java
##########
@@ -0,0 +1,114 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Trash;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.security.SecurityUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
+
+/**
+ * Background Service to empty keys that are moved to Trash.
+ */
+public class TrashDeletingService extends BackgroundService {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TrashDeletingService.class);
+
+ // Use single thread for now
+ private final static int KEY_DELETING_CORE_POOL_SIZE = 1;
+
+ private OzoneManager ozoneManager;
+
+ public void setFsConf(Configuration fsConf) {
+ this.fsConf = fsConf;
+ }
+
+ private Configuration fsConf;
+
+ public TrashDeletingService(long interval, long serviceTimeout,OzoneManager ozoneManager) {
+ super("TrashDeletingService", interval, TimeUnit.MILLISECONDS, KEY_DELETING_CORE_POOL_SIZE, serviceTimeout);
+ this.ozoneManager = ozoneManager;
+ fsConf = new Configuration();
+ }
+
+
+ @Override
+ public BackgroundTaskQueue getTasks() {
+ BackgroundTaskQueue queue = new BackgroundTaskQueue();
+ String rootPath = String.format("%s://%s/",
+ OzoneConsts.OZONE_OFS_URI_SCHEME, ozoneManager.getConfiguration().get(OZONE_OM_ADDRESS_KEY));
+ // Configuration object where the rootpath is set to an OFS Uri.
+ fsConf.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath);
+ FileSystem fs = null;
+ try {
+ fs = SecurityUtil.doAsLoginUser(
+ new PrivilegedExceptionAction<FileSystem>() {
+ @Override
+ public FileSystem run() throws IOException {
+ return FileSystem.get(fsConf);
+ }
+ });
+ } catch (IOException e) {
+ LOG.error("Cannot instantiate filesystem instance");
Review comment:
1) Please add exception trace to log like,
`LOG.error("Cannot instantiate filesystem instance", e);`
2) Should we proceed to add `TrashDeletingTask` to queue as `fs` instantiation has failed ?
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashDeletingService.java
##########
@@ -0,0 +1,114 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Trash;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.security.SecurityUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
+
+/**
+ * Background Service to empty keys that are moved to Trash.
+ */
+public class TrashDeletingService extends BackgroundService {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TrashDeletingService.class);
+
+ // Use single thread for now
+ private final static int KEY_DELETING_CORE_POOL_SIZE = 1;
+
+ private OzoneManager ozoneManager;
+
+ public void setFsConf(Configuration fsConf) {
Review comment:
I saw `setFsConf` used only in test. Please add @VisibleForTesting.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashDeletingService.java
##########
@@ -0,0 +1,114 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.hadoop.ozone.om;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Trash;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.security.SecurityUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
+
+/**
+ * Background Service to empty keys that are moved to Trash.
+ */
+public class TrashDeletingService extends BackgroundService {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TrashDeletingService.class);
+
+ // Use single thread for now
+ private final static int KEY_DELETING_CORE_POOL_SIZE = 1;
+
+ private OzoneManager ozoneManager;
+
+ public void setFsConf(Configuration fsConf) {
+ this.fsConf = fsConf;
+ }
+
+ private Configuration fsConf;
+
+ public TrashDeletingService(long interval, long serviceTimeout,OzoneManager ozoneManager) {
+ super("TrashDeletingService", interval, TimeUnit.MILLISECONDS, KEY_DELETING_CORE_POOL_SIZE, serviceTimeout);
+ this.ozoneManager = ozoneManager;
+ fsConf = new Configuration();
+ }
+
+
+ @Override
+ public BackgroundTaskQueue getTasks() {
+ BackgroundTaskQueue queue = new BackgroundTaskQueue();
+ String rootPath = String.format("%s://%s/",
+ OzoneConsts.OZONE_OFS_URI_SCHEME, ozoneManager.getConfiguration().get(OZONE_OM_ADDRESS_KEY));
+ // Configuration object where the rootpath is set to an OFS Uri.
+ fsConf.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath);
+ FileSystem fs = null;
Review comment:
would `fs` be created/instantiated for every iteration ? Can we resuse `fs` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] prashantpogde commented on a change in pull request #1507: HDDS-4307.Start Background Service for Trash Deletion in Ozone Manager
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on a change in pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#discussion_r508979580
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -197,6 +197,12 @@
public static final String OZONE_BLOCK_DELETING_SERVICE_INTERVAL_DEFAULT
= "60s";
+ public static final String OZONE_TRASH_DELETING_SERVICE_INTERVAL =
+ "ozone.trash.deleting.service.interval";
+ public static final String OZONE_TRASH_DELETING_SERVICE_INTERVAL_DEFAULT
+ = "60s";
Review comment:
wouldnt 60s default be too frequent ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] rakeshadr commented on a change in pull request #1507: HDDS-4307.Start Trash Emptier in Ozone Manager
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #1507:
URL: https://github.com/apache/hadoop-ozone/pull/1507#discussion_r512525048
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1228,17 +1238,57 @@ public void restart() throws IOException {
// Allow OM to start as Http Server failure is not fatal.
LOG.error("OM HttpServer failed to start.", ex);
}
-
omRpcServer.start();
+
isOmRpcServerRunning = true;
+ startTrashEmptier(configuration);
+
registerMXBean();
startJVMPauseMonitor();
setStartTime();
omState = State.RUNNING;
}
+
+ /**
+ * @param conf
+ * @throws IOException
+ * Starts a Trash Emptier thread that does an fs.trashRoots and performs
Review comment:
Please follow general guidelines for the javadoc.
1) Begins with function details.
2) Provide `@param` details.
3) Then `@throws` exception cases.
4) Ending with `@return` info.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1228,17 +1238,57 @@ public void restart() throws IOException {
// Allow OM to start as Http Server failure is not fatal.
LOG.error("OM HttpServer failed to start.", ex);
}
-
omRpcServer.start();
+
isOmRpcServerRunning = true;
+ startTrashEmptier(configuration);
+
registerMXBean();
startJVMPauseMonitor();
setStartTime();
omState = State.RUNNING;
}
+
+ /**
+ * @param conf
+ * @throws IOException
+ * Starts a Trash Emptier thread that does an fs.trashRoots and performs
+ * checkpointing & deletion
+ */
+ private void startTrashEmptier(Configuration conf) throws IOException {
+ long trashInterval =
+ conf.getLong(FS_TRASH_INTERVAL_KEY, FS_TRASH_INTERVAL_DEFAULT);
+ if (trashInterval == 0) {
+ return;
Review comment:
Please add a warn or even a lighter info log message to make the behavior loud to the users as this will disable trash emptier.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org