[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r443073493
##
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java
##
@@ -220,6 +224,39 @@ protected OMResponse createErrorOMResponse(
return omResponse.build();
}
+ /**
+ * Set parameters needed for return error response to client.
+ *
+ * @param omResponse
+ * @param ex - IOException
+ * @param unDeletedKeys- Set
+ * @return error response need to be returned to client - OMResponse.
+ */
+ protected OMResponse createOperationKeysErrorOMResponse(
+ @Nonnull OMResponse.Builder omResponse,
+ @Nonnull IOException ex, @Nonnull Set unDeletedKeys) {
+omResponse.setSuccess(false);
+StringBuffer errorMsg = new StringBuffer();
+errorMsg.append(exceptionErrorMessage(ex) + "\n The Keys not deleted: ");
+UnDeletedKeysResponse.Builder resp =
+UnDeletedKeysResponse.newBuilder();
+for (OmKeyInfo key : unDeletedKeys) {
+ if(key != null) {
+resp.addKeyInfo(key.getProtobuf());
+errorMsg.append(key.getObjectInfo() + "\n");
+ }
+}
+if (errorMsg != null) {
+ omResponse.setMessage(errorMsg.toString());
+}
+// TODO: Currently all delete operations in OzoneBucket.java are void. Here
+// we put the List of unDeletedKeys into Response. These KeyInfo can be
+// used to continue deletion if client support delete retry.
+omResponse.setUnDeletedKeysResponse(resp.build());
Review comment:
I think put the undeleted keys in OMResponse is good enough. Not sure if
we want to append all the key names into the string message, which can hurt the
performance when the list is huge.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r443072850
##
File path: hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto
##
@@ -856,6 +864,10 @@ message DeletedKeys {
repeated string keys = 3;
}
+message DeleteKeysResponse {
Review comment:
Usually we have a XXXRequest match with XXXResponse. Here we have
DeleteKeysRequest->DeleteKeysResponse and UnDeletedKeysResponse.
NIT: can we keep this consistent to avoid confusion for UnDeleteKeysRequest
if any in the future?
```suggestion
message DeleteKeysResponse {
repeated KeyInfo deletedKeys = 1;
repeated KeyInfo unDeletedKeys = 2;
}```
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r433519025
##
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMBatchKeyDeleteRequest.java
##
@@ -0,0 +1,226 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.request.key;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
+import org.apache.hadoop.hdds.utils.db.cache.CacheValue;
+import org.apache.hadoop.ozone.audit.AuditLogger;
+import org.apache.hadoop.ozone.audit.OMAction;
+import org.apache.hadoop.ozone.om.OMMetadataManager;
+import org.apache.hadoop.ozone.om.OMMetrics;
+import org.apache.hadoop.ozone.om.OzoneManager;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.exceptions.OMReplayException;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
+import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
+import org.apache.hadoop.ozone.om.response.OMClientResponse;
+import org.apache.hadoop.ozone.om.response.key.OMKeyDeleteResponse;
+import org.apache.hadoop.ozone.om.response.key.OMBatchKeyDeleteResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.*;
+import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer;
+import org.apache.hadoop.ozone.security.acl.OzoneObj;
+import org.apache.hadoop.util.Time;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.*;
+
+import static
org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_NOT_FOUND;
+import static
org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK;
+
+/**
+ * Handles DeleteKey request.
+ */
+public class OMBatchKeyDeleteRequest extends OMKeyRequest {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(OMBatchKeyDeleteRequest.class);
+
+ public OMBatchKeyDeleteRequest(OMRequest omRequest) {
+super(omRequest);
+ }
+
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+DeleteBatchKeyRequest deleteKeyRequest =
+getOmRequest().getDeleteBatchKeyRequest();
+Preconditions.checkNotNull(deleteKeyRequest);
+List newKeyArgsList = new ArrayList<>();
+for (KeyArgs keyArgs : deleteKeyRequest.getKeyArgsList()) {
+ newKeyArgsList.add(
+ keyArgs.toBuilder().setModificationTime(Time.now()).build());
+}
+DeleteBatchKeyRequest newDeleteKeyRequest = DeleteBatchKeyRequest
+.newBuilder().addAllKeyArgs(newKeyArgsList).build();
+
+return getOmRequest().toBuilder()
+.setDeleteBatchKeyRequest(newDeleteKeyRequest)
+.setUserInfo(getUserInfo()).build();
+ }
+
+ @Override
+ @SuppressWarnings("methodlength")
+ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
+ long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
+DeleteBatchKeyRequest deleteKeyRequest =
+getOmRequest().getDeleteBatchKeyRequest();
+
+List deleteKeyArgsList = deleteKeyRequest.getKeyArgsList();
+IOException exception = null;
+OMClientResponse omClientResponse = null;
+Result result = null;
+
+OMMetrics omMetrics = ozoneManager.getMetrics();
+omMetrics.incNumKeyDeletes();
+List omKeyInfoList = new ArrayList<>();
+Set acquiredLockSet = new HashSet<>();
+Map auditMap = null;
+String volumeName = "";
+String bucketName = "";
+String keyName = "";
+
+AuditLogger auditLogger = ozoneManager.getAuditLogger();
+OzoneManagerProtocolProtos.UserInfo userInfo =
+getOmRequest().getUserInfo();
+
+OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
+getOmRequest());
+OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+try {
+ for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+acquiredLockSet.add(deleteKeyArg
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814: URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429358962 ## File path: hadoop-hdds/common/src/main/resources/ozone-default.xml ## @@ -1966,6 +1966,14 @@ jar and false for the ozone-filesystem-lib-current.jar + +ozone.fs.iterate.batch-size +1 +OZONE, OZONEFS + + iterate batch size of delete and rename when use BasicOzoneFileSystem. Review comment: Can we update the title of the JIRA to reflect batch rename? 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: us...@infra.apache.org - To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814: URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429349020 ## File path: hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientAdapter.java ## @@ -48,7 +48,7 @@ OzoneFSOutputStream createFile(String key, short replication, boolean createDirectory(String keyName) throws IOException; - boolean deleteObject(String keyName); Review comment: Should we define a new method called deleteObjects to be backward compatible? 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: us...@infra.apache.org - To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429348515
##
File path:
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java
##
@@ -272,15 +272,15 @@ public boolean createDirectory(String keyName) throws
IOException {
/**
* Helper method to delete an object specified by key name in bucket.
*
- * @param keyName key name to be deleted
+ * @param keyNameList key name list to be deleted
* @return true if the key is deleted, false otherwise
*/
@Override
- public boolean deleteObject(String keyName) {
-LOG.trace("issuing delete for key {}", keyName);
+ public boolean deleteObject(List keyNameList) {
Review comment:
Should we define a new method called deleteObjects to be backward
compatible?
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429346155
##
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
##
@@ -187,7 +192,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager,
case SUCCESS:
omMetrics.decNumKeys();
LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,
- bucketName, keyName);
+ bucketName, keyNameList);
Review comment:
This will only print the address of the keNameList. You may want to
expand the list and also protect the LOG.debug with a if LOG.isDebugEnabled().
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429346575
##
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
##
@@ -196,7 +201,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager,
case FAILURE:
omMetrics.incNumKeyDeleteFails();
LOG.error("Key delete failed. Volume:{}, Bucket:{}, Key{}. Exception:{}",
- volumeName, bucketName, keyName, exception);
+ volumeName, bucketName, keyNameList, exception);
Review comment:
same as above. we might only want to print the key that failed in the
deletion instead of the whole list upon failures. print the whole list can be a
debug or trace level log.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429344837
##
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
##
@@ -111,51 +114,53 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
boolean acquiredLock = false;
OMClientResponse omClientResponse = null;
Result result = null;
+List omKeyInfoList= new ArrayList<>();
try {
- // check Acl
- checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
- IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
-
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
-
+ if (keyNameList.size() == 0) {
+throw new OMException("Key not found", KEY_NOT_FOUND);
+ }
acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
Review comment:
This is OK with o3fs as we will mount a single bucket. In the context of
ofs where you can have multiple volume buckets under the root. This lock can't
guarantee atomic across all the delete keyname list.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429342829
##
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
##
@@ -382,6 +382,21 @@ public void deleteKey(String key) throws IOException {
proxy.deleteKey(volumeName, name, key);
}
+ /**
+ * Deletes the given list of keys from the bucket.
+ * @param keyList List of the key name to be deleted.
+ * @throws IOException
+ */
+ public void deleteKeys(List keyList) throws IOException {
Review comment:
When we delete a list of keys, upon failure in the middle, can we return
a list of deleted keys and undeleted keys? This may not be an issue when you
delete a single key but when batch deleting, it is hard to recover from the
failures without that information.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429341429
##
File path: hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto
##
@@ -830,7 +832,7 @@ message LookupKeyResponse {
message RenameKeyRequest{
required KeyArgs keyArgs = 1;
-required string toKeyName = 2;
+optional string toKeyName = 2;
Review comment:
why do we need to change toKeyName from required to optional?
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429340218
##
File path: hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto
##
@@ -677,7 +677,7 @@ message ListBucketsResponse {
message KeyArgs {
required string volumeName = 1;
required string bucketName = 2;
-required string keyName = 3;
Review comment:
Change required to option will be an incompatible change.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #814: HDDS-3286. BasicOzoneFileSystem support batchDelete.
xiaoyuyao commented on a change in pull request #814:
URL: https://github.com/apache/hadoop-ozone/pull/814#discussion_r429339639
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java
##
@@ -256,11 +263,16 @@ public Builder setSortDatanodesInPipeline(boolean sort) {
return this;
}
+public Builder setKeyNameList(List keyList) {
+ this.keyNameList = keyList;
+ return this;
+}
+
public OmKeyArgs build() {
return new OmKeyArgs(volumeName, bucketName, keyName, dataSize, type,
Review comment:
I think other operations such as createKey still assume a single
KeyName.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
