You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2018/02/20 13:11:05 UTC
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/5535
[FLINK-8703][tests] Migrate tests from LocalFlinkMiniCluster to MiniClusterResource
## What is the purpose of the change
This PR contains the first batch of test ports from `LocalFlinkMiniCluster` to `MiniClusterResource`. No port required changes to `MiniClusterResource`.
Each test has it's own commit. If a test was not explicitly creating a configuration and the number of taskmanagers/slots matched the settings in `AbstractTestBase` the test was modified to extend that.
Otherwise, a separate `MiniClusterResource` `@ClassRule` was created.
## Verifying this change
This change is covered by existing tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink 8703
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5535.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #5535
----
commit 2b4168c932304d0a855d330cfe902b361cf7b174
Author: zentol <ch...@...>
Date: 2018-02-20T12:41:45Z
[FLINK-8703][tests] Port CollectITCase to AbstractTestBase
commit f00f69ca1eb347d4d235cb24bb1d49656548cc98
Author: zentol <ch...@...>
Date: 2018-02-20T12:42:01Z
[FLINK-8703][tests] Port MiscellaneuousIssuesITCase to AbstractTestBase
commit e6b8751b982e3b80c74b7e93ef26e60cd9059b37
Author: zentol <ch...@...>
Date: 2018-02-20T12:43:01Z
[FLINK-8703][tests] Port StreamFaultToleranceTestBase to MiniClusterResource
commit a2163be631695f78da7f40cebae1c62a3016fe17
Author: zentol <ch...@...>
Date: 2018-02-20T12:43:17Z
[FLINK-8703][tests] Port KeyedStateCheckpointITCase to MiniClusterResource
commit f85d9577df4002e98c34606984ee69573a78a6f7
Author: zentol <ch...@...>
Date: 2018-02-20T12:43:34Z
[FLINK-8703][tests] Port AccumulatorErrorITCase to AbstractTestBase
commit 06dfdb7c75298c1b501599a25e7fdae93de92c5f
Author: zentol <ch...@...>
Date: 2018-02-20T12:46:10Z
[FLINK-8703][tests] Port FastFailuresITCase to MiniClusterResource
commit 12e29e0fec70824415216ee738853d45b204fc03
Author: zentol <ch...@...>
Date: 2018-02-20T12:58:40Z
[FLINK-8703][tests] Port SimpleRecoveryITCaseBase to MiniClusterResource
----
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/5535#discussion_r169589443
--- Diff: flink-libraries/flink-gelly/src/test/java/org/apache/flink/graph/test/operations/DegreesWithExceptionITCase.java ---
@@ -38,29 +32,10 @@
* Test expected errors for {@link Graph#inDegrees()},
* {@link Graph#outDegrees()}, and {@link Graph#getDegrees()}.
*/
-public class DegreesWithExceptionITCase extends TestLogger {
+public class DegreesWithExceptionITCase extends AbstractTestBase {
private static final int PARALLELISM = 4;
--- End diff --
I think this could be replaced in all the tests by making `DEFAULT_PARALLELISM` in `AbstractTestBase` protected and using that instead.
---
[GitHub] flink issue #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkMiniClus...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5535
yes, as mentioned in PR the description this does not port all tests. There are several tests that cannot be ported in isolation as they access specific methods on the `LocalFlinkMiniCluster` or communicate directly with components via akka.
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/5535
---
[GitHub] flink issue #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkMiniClus...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/5535
JobManagerHACheckpointRecoveryITCase still use ```LocalFlinkMiniCluster``` as it's default one. And we seem to be lost refactor it. And I dont think it is a very easy work since in this ITCase, it use ```LocalFlinkMiniCluster.getLeaderGateway```. And in the new pattern of ```AbstractTestBase```. I dont know how to get that.
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5535#discussion_r169370975
--- Diff: flink-tests/src/test/java/org/apache/flink/test/misc/MiscellaneousIssuesITCase.java ---
@@ -134,7 +106,8 @@ public void testAccumulatorsAfterNoOp() {
final String accName = "test_accumulator";
try {
- env.setParallelism(6);
+ ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+ env.setParallelism(4);
--- End diff --
yeah, that's probably the better option.
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/5535#discussion_r169327476
--- Diff: flink-tests/src/test/java/org/apache/flink/test/streaming/experimental/CollectITCase.java ---
@@ -38,33 +35,23 @@
* <p>This experimental class is relocated from flink-streaming-contrib. Please see package-info.java
* for more information.
*/
-public class CollectITCase extends TestLogger {
+public class CollectITCase extends AbstractTestBase {
--- End diff --
It seems that we do not use anything in AbstractTestBase class. Why do we still extend this class ?
---
[GitHub] flink issue #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkMiniClus...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/5535
And we also seems lost refactor ```WebFrontendITCase``` which still use ```LocalFlinkMiniCluster```.
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5535#discussion_r169341512
--- Diff: flink-tests/src/test/java/org/apache/flink/test/streaming/experimental/CollectITCase.java ---
@@ -38,33 +35,23 @@
* <p>This experimental class is relocated from flink-streaming-contrib. Please see package-info.java
* for more information.
*/
-public class CollectITCase extends TestLogger {
+public class CollectITCase extends AbstractTestBase {
--- End diff --
Please have a look at the javadocs for AbstractTestBase.
The logs should still be printed as `AbstractTestBase` extends `TestBaseUtils` which extends `TestLogger`.
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/5535#discussion_r169346590
--- Diff: flink-tests/src/test/java/org/apache/flink/test/streaming/experimental/CollectITCase.java ---
@@ -38,33 +35,23 @@
* <p>This experimental class is relocated from flink-streaming-contrib. Please see package-info.java
* for more information.
*/
-public class CollectITCase extends TestLogger {
+public class CollectITCase extends AbstractTestBase {
--- End diff --
Thanks @zentol.
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/5535#discussion_r169589778
--- Diff: flink-tests/src/test/java/org/apache/flink/test/misc/SuccessAfterNetworkBuffersFailureITCase.java ---
@@ -50,22 +50,27 @@
public class SuccessAfterNetworkBuffersFailureITCase extends TestLogger {
private static final int PARALLELISM = 16;
+
+ @ClassRule
+ public static final MiniClusterResource MINI_CLUSTER_RESOURCE = new MiniClusterResource(
+ new MiniClusterResource.MiniClusterResourceConfiguration(
+ getConfiguration(),
+ 2,
--- End diff --
This could be derived from `PARALLELISM`
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/5535#discussion_r169328894
--- Diff: flink-tests/src/test/java/org/apache/flink/test/streaming/experimental/CollectITCase.java ---
@@ -38,33 +35,23 @@
* <p>This experimental class is relocated from flink-streaming-contrib. Please see package-info.java
* for more information.
*/
-public class CollectITCase extends TestLogger {
+public class CollectITCase extends AbstractTestBase {
--- End diff --
One thing I found the difference from them , If we extends AbstractTestBase class. No logs will print to console.
---
[GitHub] flink issue #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkMiniClus...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5535
I'm removing `AccumulatorErrorITCase` from the port due to FLINK-8721.
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5535#discussion_r169312549
--- Diff: flink-tests/src/test/java/org/apache/flink/test/misc/MiscellaneousIssuesITCase.java ---
@@ -134,7 +106,8 @@ public void testAccumulatorsAfterNoOp() {
final String accName = "test_accumulator";
try {
- env.setParallelism(6);
+ ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+ env.setParallelism(4);
--- End diff --
i had to reduce the parallelism to 4 as it failed with 6 due to not enough slots.
---
[GitHub] flink issue #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkMiniClus...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/5535
Thanks @zentol. Yea. I agree with you.
---
[GitHub] flink issue #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkMiniClus...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5535
will merge this, as is, once travis gives a green light. The commits have the rather nice property that they don't require changes to other classes (like the MiniClusterResource); wouldn't want to destroy that by making parallelism protected. As for deriving the parallelism, we could do it but there are in general numerous things we could clean up in these tests.
---
[GitHub] flink pull request #5535: [FLINK-8703][tests] Migrate tests from LocalFlinkM...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/5535#discussion_r169369295
--- Diff: flink-tests/src/test/java/org/apache/flink/test/misc/MiscellaneousIssuesITCase.java ---
@@ -134,7 +106,8 @@ public void testAccumulatorsAfterNoOp() {
final String accName = "test_accumulator";
try {
- env.setParallelism(6);
+ ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+ env.setParallelism(4);
--- End diff --
Why not start a mini cluster with 2 TMs and 3 slots as before?
---