[jira] [Commented] (HDDS-8) Add OzoneManager Delegation Token support

["Xiaoyu Yao (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HDDS-8?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16495853#comment-16495853 ] 

Xiaoyu Yao commented on HDDS-8:
-------------------------------

Thanks [~ajayydv] for working on this. The patch looks good to me overall. Here are are comments:

 

*KeySpaceManager.java*

*Line 188-202: we can use Mockito to mock* _getScmContainerClient and getScmBlockClient for testing without introducing the test flag._

 

_Line 269: can we use a LOG.error with ex as parameter instead here?_

 

_Line 276: we will need to open a follow up JIRA on this._

 

_Line 665: NIT: typo scret -> secret_

 

_Line 698-702: can we wrap these lines with if (LOG.isTraceEnabled())_

 

_Line 721: NIT: empty line can be removed._

 

_Line 730: unused local variable id can be removed. Or we can add a trace log with the id._

 

_Line 732: add a log entry?_

 

_Line 733-734: I noticed few empty finally{} block. If we don't catch interception or_ 

_Have final clean up work, we can remove it._

 

_Line 748:  unused local variable id can be removed. Or we can add a trace log with the id._

 

Line 1189: this can be removed with the test mock.

 

 

FSNameSystem.java

Line 5745: unnecessary change can be reverted.

 

DelegationKey.java

Line 88-109: please add followup JIRAs to change this after SCM CA key gen utils is available.

 

KeySpaceManagerProtocolClientSideTranslatorPB.java

Line 130-131: unused imports

 

 

KeySpaceManagerProtocolServerSideTranslatorPB.java

Line 133: unnecessary change

 

OzoneManagerSecretStore.java and some of the new java class file.

Need ASF license header

 

OzoneManagerSecurityUtil.java

Line 15: unused LOG

 

TestOzoneRpcClient.java

Line 361: this does not seem to test DT. If the majority of the tests are added in other place. We can remove this as unnecessary change.

 

TestSecureOzoneCluster.java

Line 373: we don't need to change the loginUser here. You can just wrap the KSM client class within

 

testUser.doAs() {

…

 

}

 

 

Can we have a short one-pager that summarize all the test cases we want? Then we can compare the added tests have enough coverage?

> Add OzoneManager Delegation Token support
> -----------------------------------------
>
>                 Key: HDDS-8
>                 URL: https://issues.apache.org/jira/browse/HDDS-8
>             Project: Hadoop Distributed Data Store
>          Issue Type: Sub-task
>          Components: Security
>            Reporter: Xiaoyu Yao
>            Assignee: Ajay Kumar
>            Priority: Major
>             Fix For: 0.3.0
>
>         Attachments: HDDS-8-HDDS-4.00.patch
>
>




--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org