You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by yanghua <gi...@git.apache.org> on 2018/06/06 16:26:55 UTC
[GitHub] flink pull request #6129: [FLINK-9503] Migrate integration tests for iterati...
GitHub user yanghua opened a pull request:
https://github.com/apache/flink/pull/6129
[FLINK-9503] Migrate integration tests for iterative aggregators
## What is the purpose of the change
*This pull request migrate integration tests for iterative aggregators*
## Brief change log
- *Migrate integration tests for iterative aggregators use `collect` api to replace temp file*
## Verifying this change
This change is already covered by existing tests, such as *AggregatorsITCase*.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / **no** / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / **not documented**)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yanghua/flink FLINK-9503
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6129.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 #6129
----
commit f3d0d8f46ccea32c827b11d35a41cbbdad8ff4f3
Author: yanghua <ya...@...>
Date: 2018-06-06T16:23:25Z
[FLINK-9503] Migrate integration tests for iterative aggregators
----
---
[GitHub] flink pull request #6129: [FLINK-9503] Migrate integration tests for iterati...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6129#discussion_r193701993
--- Diff: flink-tests/src/test/java/org/apache/flink/test/iterative/aggregators/AggregatorsITCase.java ---
@@ -117,7 +107,9 @@ public boolean filter(Long value) throws Exception {
}
}).withBroadcastSet(solution, "SOLUTION")).output(new DiscardingOutputFormat<Long>());
env.execute();
- expected = testString; // this will be a useless verification now.
+ String expected = testString; // this will be a useless verification now.
+
+ compareResultsByLinesInMemory(expected, resultPath);
--- End diff --
this is unnecessary and can be removed as the actual check is done in the function.
---
[GitHub] flink issue #6129: [FLINK-9503] Migrate integration tests for iterative aggr...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6129
@tillrohrmann this PR could be merged
---
[GitHub] flink pull request #6129: [FLINK-9503] Migrate integration tests for iterati...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on a diff in the pull request:
https://github.com/apache/flink/pull/6129#discussion_r193707442
--- Diff: flink-tests/src/test/java/org/apache/flink/test/iterative/aggregators/AggregatorsITCase.java ---
@@ -170,12 +162,12 @@ public void testAggregatorWithParameterForIterate() throws Exception {
new NegativeElementsConvergenceCriterion());
DataSet<Integer> updatedDs = iteration.map(new SubtractOneMapWithParam());
- iteration.closeWith(updatedDs).writeAsText(resultPath);
- env.execute();
+ List<Integer> result = iteration.closeWith(updatedDs).collect();
+ Collections.sort(result);
--- End diff --
@zentol the result need be sorted, otherwise the test would report error, because the result is not ordered, print this :
```
java.lang.AssertionError:
Expected :[1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 5]
Actual :[4, 2, 3, 5, 2, 5, 5, 5, 1, 3, 4, 4, 3, 5, 4]
```
---
[GitHub] flink issue #6129: [FLINK-9503] Migrate integration tests for iterative aggr...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6129
cc @zentol please review again, the reason of CI error is other problem
---
[GitHub] flink pull request #6129: [FLINK-9503] Migrate integration tests for iterati...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on a diff in the pull request:
https://github.com/apache/flink/pull/6129#discussion_r194696659
--- Diff: flink-tests/src/test/java/org/apache/flink/test/iterative/aggregators/AggregatorsITCase.java ---
@@ -170,12 +162,12 @@ public void testAggregatorWithParameterForIterate() throws Exception {
new NegativeElementsConvergenceCriterion());
DataSet<Integer> updatedDs = iteration.map(new SubtractOneMapWithParam());
- iteration.closeWith(updatedDs).writeAsText(resultPath);
- env.execute();
+ List<Integer> result = iteration.closeWith(updatedDs).collect();
+ Collections.sort(result);
--- End diff --
yes
---
[GitHub] flink pull request #6129: [FLINK-9503] Migrate integration tests for iterati...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6129#discussion_r193518671
--- Diff: flink-tests/src/test/java/org/apache/flink/test/iterative/aggregators/AggregatorsITCase.java ---
@@ -141,12 +133,17 @@ public void testAggregatorWithoutParameterForIterate() throws Exception {
new NegativeElementsConvergenceCriterion());
DataSet<Integer> updatedDs = iteration.map(new SubtractOneMap());
- iteration.closeWith(updatedDs).writeAsText(resultPath);
- env.execute();
+ List<Integer> result = iteration.closeWith(updatedDs).collect();
+ Collections.sort(result);
+
+ List<Integer> expected = new ArrayList<Integer>() {{
+ add(-3); add(-2); add(-2); add(-1); add(-1);
--- End diff --
use `Arrays.asList` instead
---
[GitHub] flink pull request #6129: [FLINK-9503] Migrate integration tests for iterati...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/6129
---
[GitHub] flink pull request #6129: [FLINK-9503] Migrate integration tests for iterati...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6129#discussion_r193702401
--- Diff: flink-tests/src/test/java/org/apache/flink/test/iterative/aggregators/AggregatorsITCase.java ---
@@ -170,12 +162,12 @@ public void testAggregatorWithParameterForIterate() throws Exception {
new NegativeElementsConvergenceCriterion());
DataSet<Integer> updatedDs = iteration.map(new SubtractOneMapWithParam());
- iteration.closeWith(updatedDs).writeAsText(resultPath);
- env.execute();
+ List<Integer> result = iteration.closeWith(updatedDs).collect();
+ Collections.sort(result);
--- End diff --
still some sort calls left.
---
[GitHub] flink pull request #6129: [FLINK-9503] Migrate integration tests for iterati...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6129#discussion_r193519270
--- Diff: flink-tests/src/test/java/org/apache/flink/test/iterative/aggregators/AggregatorsITCase.java ---
@@ -199,12 +201,17 @@ public void testConvergenceCriterionWithParameterForIterate() throws Exception {
new NegativeElementsConvergenceCriterionWithParam(3));
DataSet<Integer> updatedDs = iteration.map(new SubtractOneMap());
- iteration.closeWith(updatedDs).writeAsText(resultPath);
- env.execute();
+ List<Integer> result = iteration.closeWith(updatedDs).collect();
+ Collections.sort(result);
+
+ List<Integer> expected = new ArrayList<Integer>() {{
+ add(-3); add(-2); add(-2); add(-1); add(-1);
+ add(-1); add(0); add(0); add(0); add(0);
+ add(1); add(1); add(1); add(1); add(1);
+ }};
+ Collections.sort(expected);
--- End diff --
let's remove this so the port remains straight-forward. I don't wanna worry about these sorts changing the semantics of the test.
---
[GitHub] flink issue #6129: [FLINK-9503] Migrate integration tests for iterative aggr...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6129
hi @zentol does this PR look good to you?
---
[GitHub] flink pull request #6129: [FLINK-9503] Migrate integration tests for iterati...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6129#discussion_r194694357
--- Diff: flink-tests/src/test/java/org/apache/flink/test/iterative/aggregators/AggregatorsITCase.java ---
@@ -170,12 +162,12 @@ public void testAggregatorWithParameterForIterate() throws Exception {
new NegativeElementsConvergenceCriterion());
DataSet<Integer> updatedDs = iteration.map(new SubtractOneMapWithParam());
- iteration.closeWith(updatedDs).writeAsText(resultPath);
- env.execute();
+ List<Integer> result = iteration.closeWith(updatedDs).collect();
+ Collections.sort(result);
--- End diff --
The correct answer would've been that we may sort the result since `TestBaseUtils#compareResultsByLinesInMemory` was also sorting both `expected` and `actual`.
---
[GitHub] flink issue #6129: [FLINK-9503] Migrate integration tests for iterative aggr...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/6129
+1
---
[GitHub] flink issue #6129: [FLINK-9503] Migrate integration tests for iterative aggr...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6129
@zentol seems this PR can be merged?
---