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-4444. Remove KeyManagerImpl#refreshPipeline because it is the same as refresh(). #1593

Merged
merged 1 commit into from Dec 8, 2020

Conversation

flirmnave
Copy link
Contributor

What changes were proposed in this pull request?

Remove KeyManagerImpl#refreshPipeline() and replace with KeyManagerImpl#refresh().

What is the link to the Apache JIRA

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

How was this patch tested?

Unit test passed.

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

Can we remove refresh, and keep refreshPipeline instead, which is clear.

And also right now OmKeyInfo does not have pipelineInfo after HDDS-3658, so it is basically fetchPipeline, I am fine with leaving it as refresh also, just thought of posting here.

@ChenSammi
Copy link
Contributor

Hi @flirmnave, thanks for working on this.

The key point backed in HDDS-3658 is new API "refresh" is added in the KeyManager interface which is called from other modules. It's good to remove the duplicate functions and keep the code clean.

@flirmnave
Copy link
Contributor Author

Thanks @bharatviswa504 @ChenSammi for comment and review.

Based on @ChenSammi comment,
KeyManagerImpl#refresh() is a API which called from other modules,
so I remove KeyManagerImpl#refreshPipeline() and keep KeyManagerImpl#refresh() instead.

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

LGTM
@bharatviswa504 let's get this merged before HDDS-4537 / PR-1660?

@bharatviswa504
Copy link
Contributor

bharatviswa504 commented Dec 4, 2020

@flirmnave Then we can rename in the KeyManager to refreshPipeline, right?

But it is minor, we can go ahead with current PR.

@bharatviswa504
Copy link
Contributor

LGTM
@bharatviswa504 let's get this merged before HDDS-4537 / PR-1660?

Sure. I am fine with keeping refresh method.

@jojochuang jojochuang merged commit c215b11 into apache:master Dec 8, 2020
@flirmnave
Copy link
Contributor Author

Thanks @bharatviswa504 @ChenSammi @jojochuang for review and comment.

@flirmnave flirmnave deleted the HDDS-4444 branch December 8, 2020 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants