[
https://issues.apache.org/jira/browse/SUBMARINE-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16840404#comment-16840404
]
Peter Bacsko commented on SUBMARINE-49:
---------------------------------------
Some minor comments from me:
1. Inconsistent instantiation of {{ArrayList}}. Sometimes it's {{new
ArrayList<>();}} other times it's {{Lists.newArrayList();}}
2. {{protected void setSavedModelPath()}} - the getter is public, what's the
reason for this change?
3. {{QuickLinks}} has {{getLinks()}} method but {{Localizations}} has only
{{get()}}
4. Certain classes ({{Localizations}}, {{QuickLinks}}) are final, the rest are
not.
5. In {{RunJobParameters}}, variable {{workerParameters}} is package private.
Is it because of testing? If so, add the annotation.
6. {{RunJobParameters.parseInputPath()}} - javaDoc is incomplete
7. In {{RunJobParameters}}, some methods
({{setQuicklinks}}/{{setLocalizations}}/{{setSecurityParameters}}) are public
and annotated with {{@VisibleForTesting}}. Do they need to be public?
8. Minor: {{TensorFlowRunJobParameters.getLaunchCommands()}} if + throw is
shorter with {{Preconditions.checkState()}}.
9. {{RunJobCli}} - there's still a TODO comment
10. Ok, I know this is maybe nitpicking, but still.. so in certain classes
({{TensorBoardParameters}} / {{TestRoleResourceParser}} / {{TestLocalizations}}
/ {{TestLocalizations}} / {{TestQuickLinks}} / {{TestRoleParameters}} /
{{TestRoleParameters}} / {{TestTensorBoardParameters}}), the license string
looks a bit different:
{noformat}
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
{noformat}
There are no line feeds. They should look like this:
{noformat}
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
{noformat}
> 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, SUBMARINE-49.004.patch, SUBMARINE-49.004.patch,
> SUBMARINE-49.005.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)
