hadoop-submarine-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam Antal (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (SUBMARINE-54) Add test coverage for YarnServiceJobSubmitter and make it ready to extension for PyTorch
Date Tue, 16 Apr 2019 14:49:00 GMT

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

Adam Antal commented on SUBMARINE-54:
-------------------------------------

Thanks for the patch [~snemeth], looks like it was a great deal of effort from your side.
It looks a lot at first sight, and I only scratched the surface. Though I tried to be concise
in this review.

Thanks for the usages of the ParamBuilder, it is much more better now!

Jenkins-related stuff:
- *Compile*: pom addition is needed? (I could compile on my local without it. however I could
also compile with it.)
- *Javadoc*: 
  - Does javadoc of {{YarnServiceJobSubmitter}} state what it does now? (if not, could you
update?)
  - javadocs and stuff, also relocation of comments in the code, like in {{HadoopEnvironmentSetup#appendHdfsHome}}
(there are more of these!)
- *Checkstyle*: the usual...

Test architecture:
Since you're executing a refactor and lots of testsuites where there weren't any, you have
created lots of separate test classes as well. A few of them uses some similar properties
and same utility or other type of objects. It requires a much greater effort from your side
to see through those dozens of classes, but I'd recommend creating a structure for these classes
by decoupling base classes for the tests. These would have the same responsibilities of the
currently existing ones, but would provide those by inheritance and not by composition. It
is a much better approach regarding supportability - a good example is the hadoop-tools/hadoop-aws
where there are like 4-5 layers of testbase classes before the concrete testrun, each of those
having single responsibility for e.g. creating mock objects, providing static helper/utility
functions, etc.

To me it would be clearer to imagine this piece of code by calling {{super.setUp}} and {{super.teardown()}}
calls.
{noformat}
@Before
public void setUp() {
  fileUtils.initialize();
}
	
@After
public void teardown() throws IOException {
  fileUtils.teardown();
}
{noformat}

A few items to start:
- Consider descending from FileUtilitiesForTests, where it is used.
- Also consider the following:
||Superclass||Testclass||
|TestYarnServiceRunJobCliCommons|TestYarnServiceRunJobCliLocalization|
|AbstractLaunchCommandTestHelper|TestTensorBoardLaunchCommand|
|ComponentTestCommons|TestTensorFlowWorkerComponent|
|FileUtilitiesForTests|TestKerberosPrincipalFactory|

There are also some point that has something to do with this:
- Do you use YarnServiceUtils for mocking a static variable? I don't think it is the right
approach for this. Without javadoc I am not sure about your intention, but it look strange.
- In {{AbstractLaunchCommand}} what is the reason of this?
{noformat}
private final LaunchCommandBuilder builder;
{noformat}
{{LaunchCommandBuilder}} is a public static inner class of {{AbstractLaunchCommand}}. It makes
sense to me to make it non-static. Am I seeing it right?

General comments:
- The following classes are stateless utility classes: {{ServiceSpecFileGenerator}}, {{TensorFlowCommons}},
{{SubmarineResourceUtils}}, {{ClassPathUtilities}}, {{DockerUtilities}}, {{EnvironmentUtilities}},
{{KerberosPrincipalFactory}}, {{SubmarineResourceUtils}}, {{ZipUtilities}}.
You have not provided constructor, hence a default public constructor was provided by the
Java compiler instead. To achieve non-instantiability (quoting from Effective Java) a good
practice can be to provide a private constructor. In the constructor you can even throw Exception
to avoid construction in case of reflection.
- Some methods have messy names and have no javadoc: {{FileSystemOperations#mayDownloadAndZipIt}},
{{HadoopEnvironmentSetup #addHdfsClassPathIfNeeded}}, {{Localizer #handleLocalizations}},
{{AbstractComponent #generateAndSetLaunchCommand}}.
- In {{AbstractLaunchCommand#build}} I suggest to add incorporate the DEBUG level log into
an isDebugEnabled condition toString is called every time, even in non debug mode, which might
impact performance.

Regarding imports:
- Unused {{org.junit.Before}} in {{ComponentTestCommons}}, {{RemoteDirectoryManager}} in {{TestKerberosPrincipalFactory}}
- Wildcard import in TestKerberosPrincipalFactory (three classes are used from Assert), and
in TestServiceWrapper and TestEnvironmentUtilities: (you're only using the assertEquals from
Assert).
- Unused log import in {{TestYarnServiceRunJobCliCommons}}

Low priority stuff:
- Some more aggressive renames like 
  - storeComponentNameToLocalLaunchCommand -> storeComponentName
  - componentNameToLocalLaunchCommandMapping -> componentToLocalLaunchCommand
- constructor of {{YarnServiceJobSubmitter}} and also {{createServiceClient()}} in that class
can remain public - if they're intended to be public, then let's keep those. We may use it
elsewhere later.
- Do we need casting in {{getServiceWrapperFromJobSubmitter}}?
- I digged down a bit and we don't forbid null values at other occurrences of {{jobName}}
- is it necessary assume that it is not null?
- could you rename this: {{private String localScriptFile;}} 
  To reflect it what it does (only used for testing)?
- There were some @VisibleForTesting annotations for public functions. Are those make any
sense?
- As seen in {{TensorFlowPsComponent}}, {{Component}} class is shouting for a builder - probably
a follow-up jira can be made out of this.
- I'd move {{ParamBuilder}} into a separate class from {{TestYarnServiceRunJobCliCommons}}
- {{ZipUtilities#addDirToZip}}  try-with resources statement can be used instead of a simple
try-catch-finally:close statement.

Back-burners:
- {{FileSystemOperations#needHdfs}}: paths and Uris are converted to strings. This is not
very practical, but I don't think this should be enhanced here.
- {{getLaunchCommand}} in {{TensorFlowPsLaunchCommand}} is a bit misleading (adds an extra
'\n'). Users of this class are misled as this is not a traditional getter. Can we rename?
- In {{TensorFlowWorkerLaunchCommand.java}}:
{noformat}
Objects.requireNonNull(this.launchCommand,
        "LaunchCommand must not be null!");
{noformat}
{{StringUtils.isEmpty}} also executes nullcheck, so that part here is not needed.
- {{TensorBoardComponent#createComponent}} port 6006 can be extracted as a private static
final element (for clarity and future modification).
- I think this is not a good practice to create different log infrastructure to each component
like this {{SubmarineLogs.isVerbose()}}, but this is not the place where this should be addressed
as well.

> Add test coverage for YarnServiceJobSubmitter and make it ready to extension for PyTorch
> ----------------------------------------------------------------------------------------
>
>                 Key: SUBMARINE-54
>                 URL: https://issues.apache.org/jira/browse/SUBMARINE-54
>             Project: Hadoop Submarine
>          Issue Type: Sub-task
>            Reporter: Szilard Nemeth
>            Assignee: Szilard Nemeth
>            Priority: Major
>         Attachments: SUBMARINE-54.001.patch, SUBMARINE-54.002.patch
>
>
> This crucial class has no associated test yet. We need to improve this.



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

Mime
View raw message