Skip to content

Conversation

@cxorm
Copy link
Member

@cxorm cxorm commented Apr 3, 2020

What changes were proposed in this pull request?

Proposed change.

  • Part for testing OMGetDelegationTokenRequest
    In OMGetDelegationTokenRequest#preExecute, if the Token<OzoneTokenIdentifier> token is not null, we set UpdateGetDelegationTokenRequest with renewInterval and we set GetDelegationTokenResponse with response. So the UT test these items.

    If the Token<OzoneTokenIdentifier> token is null, we do not set GetDelegationTokenResponse with response and UT verified it.

    In OMGetDelegationTokenRequest#validateAndUpdateCache, if the token is not null, we will get OMClientResponse with non "-1" renewTime and UT verified it.

    The UT also tests validateAndUpdateCache with Token<OzoneTokenIdentifier> that causes Exception.

  • Part for testing OMGetDelegationTokenResponse
    The UT in this part tests functionality of addToDBBatch .

  • Create base classes TestOMDelegationTokenResponse and TestOMDelegationTokenRequest for later used by cancelToken and renewToken.

What is the link to the Apache JIRA

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

How was this patch tested?

Add UTs including

  • TestOMGetDelegationTokenRequest
  • TestOMGetDelegationTokenResponse

@adoroszlai adoroszlai requested a review from hanishakoneru April 8, 2020 09:47
@adoroszlai
Copy link
Contributor

@hanishakoneru since you created the task for this change, may I ask you to review?

Copy link
Contributor

@hanishakoneru hanishakoneru 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 @cxorm for working on this. Overall LGTM. Just a minor comment.

@bharatviswa504 do you wanna take a quick look as you worked on the DelegationTokenRequests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to set the secretManager again? OzoneManager.getDelegationTokenMgr is going to return the same object again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @hanishakoneru for the review.

Yes, we don't need secretManager here.
Updated.

@cxorm cxorm force-pushed the HDDS-3178 branch 2 times, most recently from 816fceb to f7b5341 Compare April 30, 2020 06:07
@hanishakoneru
Copy link
Contributor

LGTM. +1 pending CI.

@cxorm
Copy link
Member Author

cxorm commented Apr 30, 2020

Rebase latest master-branch(#839) and trigger github-actions.

@cxorm cxorm requested a review from hanishakoneru May 5, 2020 07:29
@hanishakoneru
Copy link
Contributor

Thanks @cxorm for working on this. I will merge this PR.

@hanishakoneru hanishakoneru merged commit 69ee3a3 into apache:master May 5, 2020
@cxorm
Copy link
Member Author

cxorm commented May 6, 2020

Thanks @hanishakoneru for the merge.

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.

3 participants