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 2021/02/16 03:34:06 UTC
[GitHub] [ozone] rakeshadr commented on a change in pull request #1915: HDDS-4742. Make trash work with FS Optimised Buckets.
rakeshadr commented on a change in pull request #1915:
URL: https://github.com/apache/ozone/pull/1915#discussion_r576528232
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashOzoneFileSystem.java
##########
@@ -165,14 +171,43 @@ public boolean rename(Path src, Path dst) throws IOException {
return true;
}
+ private boolean renameV1(OFSPath srcPath, OFSPath dstPath) {
+ OzoneManagerProtocolProtos.OMRequest omRequest =
+ getRenameKeyRequest(srcPath, dstPath);
+ try {
+ submitRequest(omRequest);
+ } catch (Exception e){
+ LOG.error("couldnt send rename requestV1", e);
+ }
+ return true;
+
+ }
+
@Override
public boolean delete(Path path, boolean b) throws IOException {
ozoneManager.getMetrics().incNumTrashDeletes();
+ OFSPath srcPath = new OFSPath(path);
+ OmBucketInfo bucket = ozoneManager.getBucketInfo(srcPath.getVolumeName(),
+ srcPath.getBucketName());
+ if (OzoneFSUtils.isFSOptimizedBucket(bucket)) {
+ return deleteV1(srcPath);
+ }
DeleteIterator iterator = new DeleteIterator(path, true);
iterator.iterate();
return true;
}
+ private boolean deleteV1(OFSPath srcPath) {
+ OzoneManagerProtocolProtos.OMRequest omRequest =
+ getDeleteKeyRequest(srcPath);
Review comment:
same as above comment. Do null check for the `omRequest`
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OzoneFSUtils.java
##########
@@ -205,4 +206,25 @@ public static int getFileCount(String keyName) {
java.nio.file.Path keyPath = Paths.get(keyName);
return keyPath.getNameCount();
}
+
+ /**
+ * Returns true if the bucket is FS Optimised.
+ * @param bucket
+ * @return
+ */
+ public static boolean isFSOptimizedBucket(OmBucketInfo bucket) {
+ // layout version V1 represents optimized FS path
Review comment:
@sadanand48, nice idea to add this to utility function. Can you please do a few changes to avoid duplicate functionality exists in [BasicOzoneClientAdapterImpl#L552](https://github.com/apache/ozone/blob/HDDS-2939/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L552).
Can you do changes like:
```
OzoneFSUtils.java
/**
* Returns true if the bucket is FS Optimised.
* @param buckeMetadata
* @return
*/
public static boolean isFSOptimizedBucket(Map<String, String> buckeMetadata) {
// layout version V1 represents optimized FS path
boolean layoutVersionEnabled =
org.apache.commons.lang3.StringUtils.equalsIgnoreCase(
OMConfigKeys.OZONE_OM_LAYOUT_VERSION_V1,
buckeMetadata.get(OMConfigKeys.OZONE_OM_LAYOUT_VERSION));
boolean fsEnabled =
Boolean.parseBoolean(buckeMetadata
.get(OMConfigKeys.OZONE_OM_ENABLE_FILESYSTEM_PATHS));
return layoutVersionEnabled && fsEnabled;
}
```
```
BasicOzoneClientAdapterImpl.java
@Override
public boolean isFSOptimizedBucket() {
// layout version V1 represents optimized FS path
return OzoneFSUtils.isFSOptimizedBucket(bucket.getMetadata());
}
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashOzoneFileSystem.java
##########
@@ -165,14 +171,43 @@ public boolean rename(Path src, Path dst) throws IOException {
return true;
}
+ private boolean renameV1(OFSPath srcPath, OFSPath dstPath) {
+ OzoneManagerProtocolProtos.OMRequest omRequest =
+ getRenameKeyRequest(srcPath, dstPath);
+ try {
+ submitRequest(omRequest);
+ } catch (Exception e){
+ LOG.error("couldnt send rename requestV1", e);
+ }
+ return true;
+
+ }
+
@Override
public boolean delete(Path path, boolean b) throws IOException {
ozoneManager.getMetrics().incNumTrashDeletes();
+ OFSPath srcPath = new OFSPath(path);
+ OmBucketInfo bucket = ozoneManager.getBucketInfo(srcPath.getVolumeName(),
+ srcPath.getBucketName());
+ if (OzoneFSUtils.isFSOptimizedBucket(bucket)) {
+ return deleteV1(srcPath);
+ }
DeleteIterator iterator = new DeleteIterator(path, true);
iterator.iterate();
return true;
}
+ private boolean deleteV1(OFSPath srcPath) {
+ OzoneManagerProtocolProtos.OMRequest omRequest =
+ getDeleteKeyRequest(srcPath);
+ try {
+ submitRequest(omRequest);
+ } catch (Throwable e) {
+ LOG.error("Couldn't send delete request.", e);
Review comment:
Will returning `true` gives a false positive to the user ? Is that expected behavior?
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashOzoneFileSystem.java
##########
@@ -165,14 +171,43 @@ public boolean rename(Path src, Path dst) throws IOException {
return true;
}
+ private boolean renameV1(OFSPath srcPath, OFSPath dstPath) {
+ OzoneManagerProtocolProtos.OMRequest omRequest =
+ getRenameKeyRequest(srcPath, dstPath);
+ try {
+ submitRequest(omRequest);
Review comment:
Please add a null check for omRequest. Otw, in error case NPE will get thrown and good to avoid NPE in logs.
can do something like:
```
if(omRequest != null) {
submitRequest(omRequest);.
return true;
}
return false;
```
----------------------------------------------------------------
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org