[
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: [email protected]
For additional commands, e-mail: [email protected]
