-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-5463. [FSO] Recon Container API does not work correctly with FSO. #4182
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
Conversation
|
@aryangupta1998 @devmadhuu @dombizita @sadanand48 Can you please take a look at this !! |
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
aryangupta1998
left a comment
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.
I think we don't have any usage of the 'getBucketLayout()' function as we are directly passing the bucket layout in the 'getKeyTable()' function in ContainerEndpoint.java. Can we remove 'getBucketLayout()'?
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
|
@GeorgeJahad can you please take a look as well. |
aryangupta1998
left a comment
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.
+1, LGTM!
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
|
Is there a reason why no tests have been added to confirm this fix? |
|
What about object store buckets? I know those are similar to legacy buckets but the way recon is coded they won't get handled, will they? Is that a separate PR? |
|
Hey @ArafatKhan2198 ~ there were some new comments, please help take a look. |
|
Actually, getKeyTable() returns fileTable rocksdb column family if it's FSO and returns keyTable otherwise, so no need to distinguish between OBJECT_STORE or LEGACY. But good point on the test case. We need that. |
|
@GeorgeJahad @jojochuang can you please take a look! |
6a946f2 to
f6f4220
Compare
dombizita
left a comment
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.
thanks for working on this @ArafatKhan2198, overall it looks good to me, I added a comment inline.
...op-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ContainerKeyMapperTask.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java
Outdated
Show resolved
Hide resolved
|
It would be good to add an fso test here as well: Line 153 in 94598d1
|
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java
Outdated
Show resolved
Hide resolved
1990c3c to
0c649e4
Compare
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Show resolved
Hide resolved
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.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, @ArafatKhan2198 Plz check below comments
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Show resolved
Hide resolved
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Show resolved
Hide resolved
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
|
I don't see any smoketests for this api here: https://github.com/apache/ozone/tree/master/hadoop-ozone/dist/src/main/smoketest/recon |
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
|
IMO, pseudocode,
Few optimization, generate key based on path, can see avoid getting parentId again and again |
|
@GeorgeJahad @ChenSammi @jojochuang Could you kindly review this patch? It has been open for some time. Please let me know if there are any additional changes required to expedite the merge process. Thank you. |
sumitagrawl
left a comment
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.
@ArafatKhan2198 we need have implementation wrt pagination as used by Recond UI
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
|
While working on this patch, I had a doubt regarding the endpoint Initially, we assumed that the So after a discussion this is what we have understood about the
|
|
Therefore, to conclude, there is no need for any conversion from The ContainerKey Table generated for this data would be the below table :-
|
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java
Show resolved
Hide resolved
sumitagrawl
left a comment
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.
@ArafatKhan2198 LGTM +1
What changes were proposed in this pull request?
The container Endpoint and ContainerKeyMapperTask have been updated to support both legacy and file-system optimized (FSO) buckets. Previously, only the KeyTable for legacy buckets was being referenced, but now both the KeyTable and FileTable will be utilised to fetch metadata
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5463
How was this patch tested?
Manually tested out the API along with Unit-Testing
