Szilard Nemeth commented on YARN-9997:

Hi [~gandras],
Thanks for working on this patch.

Some comments: 
1. Could you please add a javadoc to method 
Please also include the return value in the javadoc.

2. I'm not sure if it's a good idea to catch exception while deserializing the 
object in ZKConfigurationStore#logMutation. This surely modifies behaviour as 
exception is not propagating up the call chain, moreover there are some other 
operations in the method on the "logs" variable that is the deserialized object 
itself. Can you write a unit test to prove this can cause issues?

3. I know this was not included in the original description of the jira, but 
can you deal with one more thing?
In ZKConfigurationStore#getConfigVersion, IntelliJ marks the single line of 
this method with the following warning: 
"Argument zkManager.getStringData(confVersionPath) might be null". Can you 
cover this with a unit test case?


> Code cleanup in ZKConfigurationStore
> ------------------------------------
>                 Key: YARN-9997
>                 URL: https://issues.apache.org/jira/browse/YARN-9997
>             Project: Hadoop YARN
>          Issue Type: Improvement
>            Reporter: Szilard Nemeth
>            Assignee: Andras Gyori
>            Priority: Minor
>         Attachments: YARN-9997.001.patch
> Many thins can be improved:
> * znodeParentPath could be a local variable
> * zkManager could be private, VisibleForTesting annotation is not needed 
> anymore
> * Do something with unchecked casts
> * zkManager.safeSetData calls are almost having the same set of parameters: 
> Simplify this
> * Extract zkManager calls to their own methods: They are repeated
> * Remove TODOs

This message was sent by Atlassian Jira

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

Reply via email to