[
https://issues.apache.org/jira/browse/SUBMARINE-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16824133#comment-16824133
]
Adam Antal commented on SUBMARINE-49:
-------------------------------------
Thanks for the follow-up patch. I am generally happy about this patch.
- Agreed with your points as I also re-read the syntax of the parse functions.
- We're approximately on the same page, but {{RoleResourceParser}} seems to
have some kind of logic embedded. For e.g. if there are numberOfRoleInstances,
but the resourceStr is null, then it must throw an exception. Could you please
add this testcase to that?
- Got through the other tests, and they seem proper and meaningful.
- In {{RunJobParameters$validateWorkersAndPs}} maybe they should be one
sentence:
{noformat}
/**
* Check number of workers and PS instances.
* When distributed training is required.
* @param numberOfWorkers number of worker instances
* @param numberOfPS number of PS (ParameterServer) instances
* @throws ParseException
*/
{noformat}
- Though they're more or less straightforward but for sake of concreteness
could you update the following function's javadoc?
{{RunJobParameters$setDefaultDirs}}, {{getCheckPointPath}} I'm asking this
because it is a bit ambiguous to me how are the params being used (e.g. set
Default Dirs in what, how are the parameters used...).
- One wildcard import (Assert) in {{TestQuickLinks.java}} and
{{TestTensorBoardParameters.java}}.
> Add more test coverage to RunJobParameters
> ------------------------------------------
>
> Key: SUBMARINE-49
> URL: https://issues.apache.org/jira/browse/SUBMARINE-49
> Project: Hadoop Submarine
> Issue Type: Sub-task
> Reporter: Szilard Nemeth
> Assignee: Szilard Nemeth
> Priority: Minor
> Attachments: SUBMARINE-49.001.patch, SUBMARINE-49.002.patch,
> SUBMARINE-49.003.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)
