Skip to content
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-10367. Fix possible NullPointerException in listKeysLight method #6221

Merged
merged 11 commits into from
Feb 28, 2024

Conversation

ivanzlenko
Copy link
Contributor

What changes were proposed in this pull request?

Fix possible NullPointerException which can occur in some cases while invoking listKeysLight from OzoneManagerProtocolClientSideTranslatorPB.

What is the link to the Apache JIRA

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

How was this patch tested?

This patch is test manually.
Separate epic created to cover testing of hdfs client with S3A protocol: https://issues.apache.org/jira/browse/HDDS-10381

…rotocolPB/OzoneManagerProtocolClientSideTranslatorPB.java

Co-authored-by: Maksim Myskov <maxim.myskov@gmail.com>
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ivanzlenko for the patch.

Please add test case (for non-light listStatus) in TestListStatus#testSortedListStatus.

@ivanzlenko
Copy link
Contributor Author

@adoroszlai I've added additional test cases but looking at test coverage all changes were covered already. Looks like some of the issues with s3a protocol were fixed in HDDS-9762, but my fixes should prevent future cases where we accidentally could pass 'null' for startKey.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ivanzlenko for updating the patch.

@kerneltime
Copy link
Contributor

cc @tanvipenumudy

Copy link
Contributor

@myskov myskov left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ivanzlenko for restoring package in the test class.

Interestingly, the new test case passes without the added null checks, so it does not seem to exercise the code path being changed.

@ivanzlenko
Copy link
Contributor Author

@adoroszlai we have some preliminary checks for nulls in intermediary code. So this test cases here for good measure. I believe we would've caught HDDS-9762 earlier having cases with null value.

Comment on lines 2306 to 2308
if (startKey != null) {
listStatusRequestBuilder.setStartKey(startKey);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is also a ListStatusRequest, I think we need to set "" in else branch here, too.

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

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

@ivanzlenko Thanks for working over this, its fix for consistency with listKeys, but do not have any NullPointerException as startKey as null is expected value.
And for improving test cases.

@adoroszlai adoroszlai merged commit 1830fe2 into apache:master Feb 28, 2024
35 checks passed
@adoroszlai
Copy link
Contributor

Thanks @ivanzlenko for the patch, @myskov, @sumitagrawl for the review.

@ivanzlenko
Copy link
Contributor Author

@myskov @sumitagrawl @adoroszlai thanks for review!

@ivanzlenko ivanzlenko deleted the HDDS-10367 branch February 28, 2024 12:30
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Mar 15, 2024
…Light (apache#6221)

(cherry picked from commit 1830fe2)
Change-Id: Ib7da5a7886b3a3a39b900f9f3e7ba0e406547153
myskov pushed a commit to myskov/ozone that referenced this pull request Apr 3, 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