Skip to content

Conversation

@myskov
Copy link
Contributor

@myskov myskov commented Sep 10, 2022

What changes were proposed in this pull request?

This PR removes dead code from omKeyManagerImpl class. The most significant part of the removed code is helper methods of listStatusFSO(), which is unreachable and has been removed.

What is the link to the Apache JIRA

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

How was this patch tested?

Existing unit and integration tests

@myskov myskov changed the title HDDS-7211 remove dead code from org.apache.hadoop.ozone.omKeyManagerImpl HDDS-7211. remove dead code from org.apache.hadoop.ozone.omKeyManagerImpl Sep 10, 2022
@myskov myskov force-pushed the HDDS-7211_remove_dead_code branch 2 times, most recently from 6a51441 to f162f47 Compare September 11, 2022 05:09
@kerneltime
Copy link
Contributor

@neils-dev

@kerneltime
Copy link
Contributor

I think there needs to be a couple of more reviewers to approve this PR, there is a lot of code that is being deleted (which is good) but we need to be sure that it won't be missed.

Copy link
Member

@aswinshakil aswinshakil left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @myskov. I remember there was a plan to do a cleanup patch of listStatusFSO() after this patch but wasn't done. I can approve it once the merge conflicts are resolved.

@myskov myskov force-pushed the HDDS-7211_remove_dead_code branch 4 times, most recently from 7cbfcfb to 615d168 Compare October 4, 2022 05:44
@myskov myskov force-pushed the HDDS-7211_remove_dead_code branch from 615d168 to f176017 Compare October 4, 2022 09:03
@myskov
Copy link
Contributor Author

myskov commented Oct 4, 2022

@aswinshakil I resolved all conflicts.

@myskov myskov requested a review from aswinshakil October 4, 2022 10:58
@myskov
Copy link
Contributor Author

myskov commented Oct 6, 2022

since @aswinshakil hasn't been active for the last week, could you take a look at the PR please @kerneltime @adoroszlai ?

@adoroszlai adoroszlai requested review from adoroszlai and aswinshakil and removed request for aswinshakil October 6, 2022 17:22
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 @myskov for the cleanup, LGTM. We can wait for @aswinshakil to take another look before merging this.

Copy link
Member

@aswinshakil aswinshakil left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup @myskov. Approved it. For future reference, Please avoid force pushing as it makes it harder to review incremental changes.

@adoroszlai adoroszlai merged commit 4e36400 into apache:master Oct 6, 2022
@adoroszlai
Copy link
Contributor

Thanks @myskov for the patch, @aswinshakil, @kerneltime for the review.

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.

4 participants