Skip to content

Conversation

@tisonkun
Copy link
Member

@tisonkun tisonkun commented Oct 24, 2018

What is the purpose of the change

Remove legacy test YARNSessionFIFOITCase#testJavaAPI. Since the functions are covered by DefaultCLITest

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with (Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

cc @tillrohrmann

@zentol
Copy link
Contributor

zentol commented Oct 24, 2018

14:45:49.851 [INFO] There are 3 errors reported by Checkstyle 8.9 with /tools/maven/checkstyle.xml ruleset.
14:45:49.852 [ERROR] src/test/java/org/apache/flink/yarn/YARNITCase.java:[127] (javadoc) JavadocType: Missing a Javadoc comment.
14:45:49.852 [ERROR] src/test/java/org/apache/flink/yarn/YARNSessionFIFOITCase.java:[38] (imports) ImportOrder: Import org.apache.flink.yarn.util.YarnTestUtils appears after other imports that it should precede
14:45:49.852 [ERROR] src/test/java/org/apache/flink/yarn/YARNSessionFIFOITCase.java:[39] (imports) ImportOrder: 'org.apache.hadoop.fs.Path' should be separated from previous imports.

@tisonkun
Copy link
Member Author

@zentol thanks for the review, add a hotfix :-)

@zentol
Copy link
Contributor

zentol commented Oct 25, 2018

20:26:56.337 [INFO] --- maven-checkstyle-plugin:2.17:check (validate) @ flink-yarn-tests_2.11 ---
20:26:56.533 [INFO] There is 1 error reported by Checkstyle 8.9 with /tools/maven/checkstyle.xml ruleset.
20:26:56.534 [ERROR] src/test/java/org/apache/flink/yarn/YARNITCase.java:[31,8] (imports) UnusedImports: Unused import: org.apache.flink.streaming.api.functions.source.ParallelSourceFunction.

@tisonkun
Copy link
Member Author

@zentol thanks and sorry I should have paid more attention

@tisonkun
Copy link
Member Author

@zentol I have resolved travis issue. Could you review this when you're free? There are little changes though.

@tisonkun
Copy link
Member Author

cc @GJL would you like to give a glance at this thread? it's not quite complex imo.

@GJL GJL self-assigned this Jan 15, 2019
* Test the YARN Java API.
*/
@Test
public void testJavaAPI() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we delete testJavaAPI? This test only starts a session cluster and tests whether the ClusterConnectionInfo and the URL of the web UI are not null. I don't see how this is related to Yarn's FIFO scheduler, i.e., the test does not belong here. Retrieving the ClusterConnectionInfo and URL of the web ui is already covered by other tests [1][2]. Lastly, the ported version in this PR does not have the same assertions as before. Let me know what you think, @tisonkun.

[1]

final LeaderConnectionInfo clusterConnectionInfo = clusterClient.getClusterConnectionInfo();

[2] https://github.com/apache/flink/blob/0a54983ca0207444477cfe991dbee1b611496204/flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your investigation. I've checkout it and agree with you, adding a follow-up to address your comment.

As a following consideration, we should properly test FLINK on YARN in the future. Current status is not quite robust and elegant IMO (ó﹏ò。)

@tisonkun tisonkun changed the title [FLINK-10665] [tests] Port YARNSessionFIFOITCase#testJavaAPI to new c… [FLINK-10665] [tests] Remove legacy test YARNSessionFIFOITCase#testJavaAPI Jan 17, 2019
GJL pushed a commit to GJL/flink that referenced this pull request Jan 18, 2019
@asfgit asfgit closed this in 3cf593c Jan 18, 2019
@tisonkun tisonkun deleted the FLINK-10665 branch January 18, 2019 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants