[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix
YARNITCase random fail
URL: https://github.com/apache/flink/pull/9175#discussion_r307368991
##
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java
##
@@ -573,6 +576,23 @@ public static int getRunningContainers() {
return count;
}
+ void waitUntilApplicationFinished(ApplicationId applicationId, Duration
timeout) throws Exception {
+ Deadline deadline = Deadline.now().plus(timeout);
+ while (true) {
Review comment:
agreed
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix
YARNITCase random fail
URL: https://github.com/apache/flink/pull/9175#discussion_r307368911
##
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java
##
@@ -573,6 +576,23 @@ public static int getRunningContainers() {
return count;
}
+ void waitUntilApplicationFinished(ApplicationId applicationId, Duration
timeout) throws Exception {
+ Deadline deadline = Deadline.now().plus(timeout);
+ while (true) {
+ YarnApplicationState state =
yarnClient.getApplicationReport(applicationId).getYarnApplicationState();
+ if (state == YarnApplicationState.FINISHED) {
+ break;
+ } else if (state == YarnApplicationState.FAILED ||
state == YarnApplicationState.KILLED) {
+ Assert.fail("Application became FAILED or
KILLED while expecting FINISHED");
+ } else {
+ sleep(sleepIntervalInMS);
+ }
+ if (deadline.isOverdue()) {
+ Assert.fail("Application didn't finish before
timeout");
Review comment:
agreed
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix
YARNITCase random fail
URL: https://github.com/apache/flink/pull/9175#discussion_r307368278
##
File path: flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNITCase.java
##
@@ -117,17 +122,35 @@ public void testPerJobMode() throws Exception {
assertThat(jobResult,
is(notNullValue()));
assertThat(jobResult.getSerializedThrowable().isPresent(), is(false));
-
waitUntilApplicationFinished(applicationId, yarnAppTerminateTimeout);
+
waitApplicationFinishedElseKillIt(applicationId, yarnAppTerminateTimeout,
yarnClusterDescriptor);
} finally {
if (clusterClient != null) {
clusterClient.shutdown();
}
-
- if (applicationId != null) {
-
yarnClusterDescriptor.killCluster(applicationId);
- }
}
}
});
}
+
+ private void waitApplicationFinishedElseKillIt(
+ ApplicationId applicationId,
+ Duration timeout,
+ YarnClusterDescriptor yarnClusterDescriptor) throws
Exception {
+ Deadline deadline = Deadline.now().plus(timeout);
+ YarnApplicationState state =
yarnClient.getApplicationReport(applicationId).getYarnApplicationState();
+
+ while (state != YarnApplicationState.FINISHED) {
+ if (state == YarnApplicationState.FAILED || state ==
YarnApplicationState.KILLED) {
+ Assert.fail("Application became FAILED or
KILLED while expecting FINISHED");
+ } else {
+ sleep(sleepIntervalInMS);
+ }
+ if (deadline.isOverdue()) {
+
yarnClusterDescriptor.killCluster(applicationId);
+ Assert.fail("Application didn't finish before
timeout");
+ }
+ state =
yarnClient.getApplicationReport(applicationId).getYarnApplicationState();
Review comment:
nit: I would put `sleep(sleepIntervalInMS);` before `state = ...` so that it
does not get lost accidentally in if..else branching.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail URL: https://github.com/apache/flink/pull/9175#discussion_r307365773 ## File path: flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java ## @@ -163,7 +160,7 @@ protected static File yarnSiteXML = null; - private YarnClient yarnClient = null; + protected YarnClient yarnClient = null; Review comment: We already have a getter for the `yarnClient` which we use in the `YARNITCase.testPerJobMode`. The same `getYarnClient` can be used in `waitApplicationFinishedElseKillIt` then there is no need for this `protected` change. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix
YARNITCase random fail
URL: https://github.com/apache/flink/pull/9175#discussion_r306933213
##
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java
##
@@ -573,6 +576,23 @@ public static int getRunningContainers() {
return count;
}
+ void waitUntilApplicationFinished(ApplicationId applicationId, Duration
timeout) throws Exception {
+ Deadline deadline = Deadline.now().plus(timeout);
+ while (true) {
+ YarnApplicationState state =
yarnClient.getApplicationReport(applicationId).getYarnApplicationState();
+ if (state == YarnApplicationState.FINISHED) {
+ break;
+ } else if (state == YarnApplicationState.FAILED ||
state == YarnApplicationState.KILLED) {
+ Assert.fail("Application became FAILED or
KILLED while expecting FINISHED");
+ } else {
+ sleep(sleepIntervalInMS);
+ }
+ if (deadline.isOverdue()) {
+ Assert.fail("Application didn't finish before
timeout");
Review comment:
I think we should move the app killing from the finally block of
`YARNITCase. waitUntilApplicationFinished` to this timeout case, right before
`Assert.fail("Application didn't finish before timeout");`. As we need killing
only if the app hangs but we do not need if it finished successfully.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix
YARNITCase random fail
URL: https://github.com/apache/flink/pull/9175#discussion_r306932028
##
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java
##
@@ -573,6 +576,23 @@ public static int getRunningContainers() {
return count;
}
+ void waitUntilApplicationFinished(ApplicationId applicationId, Duration
timeout) throws Exception {
+ Deadline deadline = Deadline.now().plus(timeout);
+ while (true) {
Review comment:
I would prefer
```
YarnApplicationState state;
do {
state =
yarnClient.getApplicationReport(applicationId).getYarnApplicationState();
.
} while (state != YarnApplicationState.FINISHED);
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix
YARNITCase random fail
URL: https://github.com/apache/flink/pull/9175#discussion_r306227658
##
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java
##
@@ -573,6 +574,20 @@ public static int getRunningContainers() {
return count;
}
+ public void waitUntilApplicationFinished(ApplicationId applicationId,
int timeout) throws Exception {
Review comment:
the method can be also package private
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix
YARNITCase random fail
URL: https://github.com/apache/flink/pull/9175#discussion_r306227369
##
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java
##
@@ -573,6 +574,20 @@ public static int getRunningContainers() {
return count;
}
+ public void waitUntilApplicationFinished(ApplicationId applicationId,
int timeout) throws Exception {
+ Deadline deadline =
Deadline.now().plus(Duration.ofSeconds(timeout));
+ while (deadline.hasTimeLeft()) {
+ YarnApplicationState state =
yarnClient.getApplicationReport(applicationId).getYarnApplicationState();
+ if (state == YarnApplicationState.FINISHED ||
+ state == YarnApplicationState.FAILED ||
+ state == YarnApplicationState.KILLED) {
+ break;
+ } else {
+ sleep(500);
Review comment:
also a private static final constant_milli
I think 100 ms is actually enough to wait in most of normal cases.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix
YARNITCase random fail
URL: https://github.com/apache/flink/pull/9175#discussion_r306226295
##
File path: flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNITCase.java
##
@@ -113,6 +113,8 @@ public void testPerJobMode() throws Exception {
assertThat(jobResult,
is(notNullValue()));
assertThat(jobResult.getSerializedThrowable().isPresent(), is(false));
+
+
waitUntilApplicationFinished(applicationId, 10);
Review comment:
Could you move `10` to a named private static final constant?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix YARNITCase random fail
azagrebin commented on a change in pull request #9175: [FLINK-12038] [test] fix
YARNITCase random fail
URL: https://github.com/apache/flink/pull/9175#discussion_r306227023
##
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java
##
@@ -573,6 +574,20 @@ public static int getRunningContainers() {
return count;
}
+ public void waitUntilApplicationFinished(ApplicationId applicationId,
int timeout) throws Exception {
Review comment:
I would suggest to rename `timeout` to `timeoutSec` or better make it of
`Duration` type instead of `int`.
Then also a private static final constant can be of the `Duration` type in
the previous comment.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
