HDDS-4581. Cleanup usage of volumeArgs in KeyRequests. #1693
Conversation
linyiqun
left a comment
linyiqun
left a comment
There was a problem hiding this comment.
Few minor comments from me.
In additional, with compared the change in HDD-4308. S3MultipartUploadAbortRequest/TestS3MultipartUploadAbortRequest is missed updated here.
| .collect(Collectors.toList()); | ||
| omKeyInfo.appendNewBlocks(newLocationList, false); | ||
|
|
||
| omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName); |
There was a problem hiding this comment.
Why we not remove omVolumeArgs variable defined in this method?
There was a problem hiding this comment.
I missed it, thanks for catching it.
| long scmBlockSize = ozoneManager.getScmBlockSize(); | ||
| int factor = omKeyInfo.getFactor().getNumber(); | ||
| omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName); | ||
| omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName); |
There was a problem hiding this comment.
Same comment like above.
|
Thank You @linyiqun for the review, addressed review comments in the latest update. |
|
@bharatviswa504 , would you mind checking if the failed unit test related to this PR? If not, I am +1 for this PR. |
captainzmc
left a comment
captainzmc
left a comment
There was a problem hiding this comment.
LGTM over all, just one comment. CI failure may have nothing to do with this PR and we can retrigger it.
| .collect(Collectors.toList()); | ||
| omKeyInfo.appendNewBlocks(newLocationList, false); | ||
|
|
||
| omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName); |
There was a problem hiding this comment.
All the use of the getVolumeInfo method has been removed, so getVolumeInfo should not be needed, we can delete it.
|
Thanks for @bharatviswa504’s patch. Also thanks for the review of @linyiqun. Merged this. |
3 participants
What changes were proposed in this pull request?
Cleanup usage of VolumeArgs in Request/Response classes in Key requests.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4581
How was this patch tested?
Existing tests.