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

Xiaoyu Yao commented on HDDS-120:
---------------------------------

Thanks [~dineshchitlangia] for working on this. Patch v2 looks pretty good to 
me overall. Here are a few minor comments:

 

*BlockData.java*

 

Line 272-275: can be simplified with .append(blockID.toString())

 

Line 285-291: this is not necessary as castChunkList() already has similar 
logic.

 

Line 297: can we bump up the log level to debug?

 

*ContainerCommandRequestPBHelper.java*

Line 43: should we use switch against msg.getCmdType() instead of the 
hard-coded CmdType number?

This way, the code is more readable and will not need to be changed when 
protobuf changes.

 

Line 45: NIT: can we have a local variable like below before the switch 
statement and use it in the cases?

String containerID = String.valueOf(msg.getContainerID()? 

 

Can we add containerType into auditParams for CreateContainer?

 

Line 53: can we add forceUdpate into auditParams for UpdateContainer?

 

Line 71: NIT: we might get a lot of chunk info for large file, which might not 
be desirable.

Maybe we should have a option to disable audit log chunk info by default.

 

Line 155: given this is for put small file, I think it should be OK to include 
the chunkInfo here in the audit log.

 

Line 180: can we add a debug log for unknown cmd type?

 

*HddsDispatcher.java*

Line 71/72: can we make this a enum?

 

Line 81: we may not need dnActionMap, DNAction class can provide a method to 
parse the enum of CmdType and convert it to DNAction directly.

 

Line 125: we should avoid using hardcoded enum numbers. 

 

Line 132: can you elaborate on the usage of shouldAuditFailure and the logic 
around 194-200? 

I see it is set as false but line 145/153/163 still audit log failures.

 

Currently, only the createContainer is audited. Do you intend to add others in 
later patches?

 

*Ozone*

Line 71: NIT: can we rename HDFS_DN_OPTS to HDDS_DN_OPTS?

> Adding HDDS datanode Audit Log
> ------------------------------
>
>                 Key: HDDS-120
>                 URL: https://issues.apache.org/jira/browse/HDDS-120
>             Project: Hadoop Distributed Data Store
>          Issue Type: Sub-task
>            Reporter: Xiaoyu Yao
>            Assignee: Dinesh Chitlangia
>            Priority: Major
>              Labels: alpha2
>         Attachments: HDDS-120.001.patch, HDDS-120.002.patch
>
>
> This can be useful to find users who overload the DNs. 



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

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

Reply via email to