Skip to content

Conversation

@sadanand48
Copy link
Contributor

@sadanand48 sadanand48 commented Jan 23, 2024

What changes were proposed in this pull request?

Fix cases when the keyPrefix pertains to a particular file/dir . For more details on the problem check jira description.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9065

How was this patch tested?

Unit tests.

@sadanand48 sadanand48 marked this pull request as ready for review January 23, 2024 17:35
@sadanand48
Copy link
Contributor Author

@whbing would you like to take a look?

@whbing
Copy link
Contributor

whbing commented Jan 25, 2024

@sadanand48 Thanks for working on this ! Overall looks good.
Since this issue has been around for a long time, a similar method listStatusLight has been added , and I'm not sure if there is a similar problem in listStatusLight. Such as follow:

List<OzoneFileStatusLight> statuses = proxy.listStatusLight(volumeName,
name, keyPrefix, false, startKey, listCacheSize, true);

this.omDefaultReplication = omDefaultReplication;
}

@SuppressWarnings("methodlength")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: To avoid reaching methodlength, perhaps we can organize the code a bit. We can eliminate some unnecessary line breaks because there is no longer an 80-character line limit. Such as the following.

final String bucketKey = metadataManager.getBucketKey(volumeName,
bucketName);
final OmVolumeArgs volumeInfo = metadataManager.getVolumeTable()
.get(volumeKey);
final OmBucketInfo omBucketInfo = metadataManager.getBucketTable()
.get(bucketKey);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a good tool which can help us format this project?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe even can be split into several methods (validate parameters, handle different cases, collect results, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks , I have refactored the method now.

@sadanand48
Copy link
Contributor Author

I'm not sure if there is a similar problem in listStatusLight

Thanks for the review @whbing , listStatusLight calls listStatus internally, It only trims the keyInfo object to be sent over the wire.

@hemantk-12 hemantk-12 requested a review from swamirishi January 30, 2024 23:31
Copy link
Contributor

@aryangupta1998 aryangupta1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aryangupta1998
Copy link
Contributor

Thanks @sadanand48 for the patch, @TaiJuWu, @adoroszlai, and @whbing for the reviews!

@aryangupta1998 aryangupta1998 merged commit 0fac57a into apache:master Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants