You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by rmetzger <gi...@git.apache.org> on 2016/10/11 13:55:56 UTC
[GitHub] flink pull request #2622: [FLINK-3706] Fix YARN test instability
GitHub user rmetzger opened a pull request:
https://github.com/apache/flink/pull/2622
[FLINK-3706] Fix YARN test instability
The most important change in this commit is that the `YarnTestBase.Runner` doesn't do `try {} catch (Throwable t) { fail(t); }` anymore, which doesn't lead to a test failure, because its called outside the main thread.
With the change, all throwables are reported back to the main thread and fail the test there properly (many YARN tests benefit from this change).
@mxm could you quickly review the changes? The change is rather small, I was considering to push a hotfix, but decided to do a quick PR review round.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rmetzger/flink flink3706
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2622.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 #2622
----
commit 7a978a57e4f5845660cff386eb629193dab44109
Author: Robert Metzger <rm...@apache.org>
Date: 2016-10-10T15:04:16Z
[FLINK-3706] Fix YARN test instability
The most important change in this commit is that the `YarnTestBase.Runner` doesn't do "try {} catch (Throwable t) { fail(t); }" anymore, which doesn't lead to a test failure, because its called outside the main thread.
With the change, all throwables are reported back to the main thread and fail the test there properly (many YARN tests benefit from this change).
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2622: [FLINK-3706] Fix YARN test instability
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2622#discussion_r82809384
--- Diff: flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java ---
@@ -599,15 +602,23 @@ protected void runWithArgs(String[] args, String terminateAfterString, String[]
break;
}
}
+ if(runner.getRunnerError() != null) {
+ break; // leave loop to send output and fail with error.
+ }
}
while (!expectedStringSeen && System.currentTimeMillis() < deadline);
--- End diff --
How about removing 605-607 and changing 609 to:
```java
while (runner.getRunnerError() == null && !expectedStringSeen && System.currentTimeMillis() < deadline);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2622: [FLINK-3706] Fix YARN test instability
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2622
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2622: [FLINK-3706] Fix YARN test instability
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2622#discussion_r82810425
--- Diff: flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java ---
@@ -670,11 +684,13 @@ public void run() {
throw new RuntimeException("Unknown type " + type);
}
- if (returnValue != 0) {
- Assert.fail("The YARN session returned with non-null value=" + returnValue);
+ if (returnValue != this.expectedReturnValue) {
+ Assert.fail("The YARN session returned with unexpected value=" + returnValue + " expected=" + expectedReturnValue);
--- End diff --
This check is duplicate because it is already performed in the `Runner`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2622: [FLINK-3706] Fix YARN test instability
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2622
We should backport the fix to the `release-1.1` branch as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2622: [FLINK-3706] Fix YARN test instability
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2622#discussion_r82809975
--- Diff: flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java ---
@@ -599,15 +602,23 @@ protected void runWithArgs(String[] args, String terminateAfterString, String[]
break;
}
}
+ if(runner.getRunnerError() != null) {
+ break; // leave loop to send output and fail with error.
+ }
}
while (!expectedStringSeen && System.currentTimeMillis() < deadline);
sendOutput();
+
+ if(runner.getRunnerError() != null) {
+ // this lets the test fail.
+ throw new RuntimeException("Runner failed", runner.getRunnerError());
+ }
Assert.assertTrue("During the timeout period of " + START_TIMEOUT_SECONDS + " seconds the " +
"expected string did not show up", expectedStringSeen);
// check for 0 return code
- Assert.assertEquals("Expected return value", returnCode, runner.getReturnValue());
+ Assert.assertEquals("Expected return value", expectedReturnValue, runner.getReturnValue());
--- End diff --
This check is duplicate because it is already performed in the `Runner`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2622: [FLINK-3706] Fix YARN test instability
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2622
Thank you for your review. I've addressed your comments and pushed a commit to Travis.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2622: [FLINK-3706] Fix YARN test instability
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2622#discussion_r82809706
--- Diff: flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java ---
@@ -621,13 +632,16 @@ protected static void sendOutput() {
public static class Runner extends Thread {
private final String[] args;
+ private final int expectedReturnValue;
private int returnValue;
--- End diff --
You can remove this field now and convert it into a local variable in the `run()` method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---