[
https://issues.apache.org/jira/browse/SUBMARINE-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16813358#comment-16813358
]
Adam Antal commented on SUBMARINE-49:
-------------------------------------
Hi [~snemeth], thanks for the patch! Awesome.
Some minor comments to start with:
* Please refrain from putting TODOs into the code. If necessary, create
separate jira.
* Add description (javadoc) to {{RoleResourceParser}}.
* It looks to me that you have created lots of {{...Parameters}} class that
has some common properties (like the parse() function, which has even similar
syntax). Though I'm not gone through the patch in depth, but its a red flag for
me, showing that it may have some common behaviour that can be moved into a
superclass. What do you think about this?
* I think some unit tests should be added to these {{...Parameters}} class.
I'm foreseeing lots of mocking, but IMO it's important.
* A great deal of new tests in {{TestRunJobParameters.java}}: yay, that's what
I call *testing*!
but also at some point the new tests are using some internal methods that
needs some javadoc.
* You have modified the expected error msg in
{{testNoInputPathOptionSpecified}}. Did that unit test fail before?
* What does the nPS abbrevation mean? I think it would help the reader of the
class to understand the code better, if we're doing some simplifying here.
Some formal stuff: inconsistent indentation (probably checkstyle gonna yell
about that)
{noformat}
public Resource parseResource(int numberOfRoleInstances,
String resourceKey, String resourceStr)
{noformat}
but few lines after:
{noformat}
private Resource parseResourceInternal(int numberOfRoleInstances,
String resourceKey, String resourceStr)
{noformat}
I did not managed to get through the whole patch, so I'll take another round
later. Thanks again for the work!
> Add more test coverage to RunJobParameters
> ------------------------------------------
>
> Key: SUBMARINE-49
> URL: https://issues.apache.org/jira/browse/SUBMARINE-49
> Project: Hadoop Submarine
> Issue Type: Improvement
> Reporter: Szilard Nemeth
> Assignee: Szilard Nemeth
> Priority: Minor
> Attachments: SUBMARINE-49.001.patch
>
>
> There are some good tests in
> {{org.apache.hadoop.yarn.submarine.client.cli.TestRunJobCliParsing}}, but
> these are not testing all fields set by method
> {{org.apache.hadoop.yarn.submarine.client.cli.param.RunJobParameters#updateParametersByParsedCommandline}}.
>
> Some more extensive testing is needed in this area.
> As an added bonus, the code
> {{org.apache.hadoop.yarn.submarine.client.cli.param.RunJobParameters#updateParametersByParsedCommandline}}
> could be cleaned up a bit.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
