New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-1054. List Multipart uploads in a bucket #1277
Conversation
...e/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmMultipartUploadCompleteList.java
Outdated
Show resolved
Hide resolved
In the list, we have different parameters like key-marker, max-uploads and few other parameters. |
Yes. I pagination is not yet implemented. This is the minimal implementation to support docker registry. Will create an other jira to add all the pagination magic. |
We need to rebase this patch since we have removed the Rest Client. Thanks. |
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneMultipartUploadList.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneMultipartUploadList.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java
Outdated
Show resolved
Hide resolved
metadataManager.getOpenKeyTable(); | ||
|
||
OmKeyInfo omKeyInfo = | ||
openKeyTable.get(upload.getDbKey()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we are reading openKeyTable only for getting creation time. If we can have this information in omMultipartKeyInfo, we could avoid DB calls for openKeyTable.
To do this, We can set creationTime in OmMultipartKeyInfo during initiateMultipartUpload . In this way, we can get all the required information from the MultipartKeyInfo table.
And also StorageClass is missing from the returned OmMultipartUpload, as listMultipartUploads shows StorageClass information. For this, if we can return replicationType and depending on this value, we can set StorageClass in the listMultipartUploads Response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are 100% right. But it seems to be a bigger change. Let's do it in https://issues.apache.org/jira/browse/HDDS-2131
(On the other hand I added audit + metrics support because they were just a few lines)
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmMultipartUpload.java
Outdated
Show resolved
Hide resolved
public OmMultipartUploadList listMultipartUploads(String volumeName, | ||
String bucketName, String prefix) throws OMException { | ||
Preconditions.checkNotNull(volumeName); | ||
Preconditions.checkNotNull(bucketName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefix also should not be null. As prefix is also required in ListMultipartUploadRequest in proto.
And also here we using "+" for concatentation, so if we pass null for prefix, then it will be /volume/bucket/null. The below method is called by getMultipartUploadKeys.
public static String getDbKey(String volume, String bucket, String key) {
return OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket +
OM_KEY_PREFIX + key;
}
When i run the command I see below output, whereas it is not showing up bucketName, keyMarker other fields.
|
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmMultipartUploadList.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have few comments in place.
Can we open Jira's for all TODO's in this Jira for tracking purpose?
- Audit support for the new method.
- Pagination support and other query parameters support.
- If replication type will not be handled in this Jira, can you open Jira for this one too.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
Show resolved
Hide resolved
OmMultipartUpload.getDbKey(volumeName, bucketName, prefix); | ||
iterator.seek(prefixKey); | ||
|
||
while (iterator.hasNext()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we need to consider table cache also now HA/Non-HA code path is merged (HDDS-1909)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please help me to understand how should it be done? What about if an MPU is finished and deleted? How is it cached? I think I can't return with (cached values + db values) because the deletions....
Sure. Audit + the replication type support is added to this patch. I created HDDS-2130 for the pagination support. |
💔 -1 overall
This message was automatically generated. |
/retest |
2 similar comments
/retest |
/retest |
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmMultipartUploadList.java
Outdated
Show resolved
Hide resolved
public static S3StorageType fromReplicationType( | ||
ReplicationType replicationType) { | ||
if (replicationType == ReplicationType.STAND_ALONE) { | ||
return S3StorageType.REDUCED_REDUNDANCY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a question, I think this is before even this patch.
Previously we use STAND_ALONE for replication factor one, now we use RATIS with one and three. So, I think we need to change the code for this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable but it requires the change of the protobuf (factor is not available from the message). Seems to be a bigger change but I added it to this patch (sorry, now it's an other commit to review :-P )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank You @elek for the updated PR.
Mostly looks good to me. Few minor comments/questions.
And there are few acceptance test failures. Mostly looks unrelated, can you verify them.
/retest |
int nextMarker, boolean truncate) { | ||
this.replicationType = type; | ||
this.replicationFactor = factor; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace:end of line
💔 -1 overall
This message was automatically generated. |
...one-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have one minor comment.
💔 -1 overall
This message was automatically generated. |
+1 LGTM. |
This Jira is to implement in ozone to list of in-progress multipart uploads in a bucket.
[https://docs.aws.amazon.com/AmazonS3/latest/API/mpUploadListMPUpload.html]
See: https://issues.apache.org/jira/browse/HDDS-1054