You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "Alisha-Anaum (via GitHub)" <gi...@apache.org> on 2023/07/19 04:15:40 UTC
[GitHub] [incubator-uniffle] Alisha-Anaum opened a new pull request, #1023: Fix the flaky test GrpcServerTest
Alisha-Anaum opened a new pull request, #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023
### What changes were proposed in this pull request?
I observed a previous fix injected some delay to fix the flaky test, but I believe that fix might be unstable in a CI environment or when run on different machines, given the dependency on some constant wait time. I suggest a new way to fix the test by adding some synchronization for the test execution only. I at first identify the source code location whose slow execution leads to the flaky test failure, where if common/src/main/java/org/apache/uniffle/common/rpc/GrpcServer.java#157 slows the grpcMetrics.incGauge() and grpcMetrics.setGauge(), then the test fails (https://github.com/apache/incubator-uniffle/blob/master/common/src/main/java/org/apache/uniffle/common/rpc/GrpcServer.java#L157 and https://github.com/apache/incubator-uniffle/blob/master/common/src/main/java/org/apache/uniffle/common/rpc/GrpcServer.java#L158). Hence, I introduce one variable in this class [GrpcServer.java](https://github.com/apache/incubator-uniffle/blob/master/common/src/main/java/org/apache/uniffle/co
mmon/rpc/GrpcServer.java#L158) that is only there to provide some synchronization. Basically, until these two statements are executed, I force the thread that the test runs on to wait before it accesses the value of grpcMetrics.getGaugeMap(). The waiting location is at https://github.com/apache/incubator-uniffle/blob/master/common/src/test/java/org/apache/uniffle/common/rpc/GrpcServerTest.java#L72
### Why are the changes needed?
This test is flakily fails. I run this test many times and it makes assertion fails. The failure message is as follows.
Failure:
[INFO] Running org.apache.uniffle.common.rpc.GrpcServerTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.185 s <<< FAILURE! - in org.apache.uniffle.common.rpc.GrpcServerTest
[ERROR] testGrpcExecutorPool Time elapsed: 0.142 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <2.0> but was: <0.0>
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:70)
at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:65)
at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:885)
at org.apache.uniffle.common.rpc.GrpcServerTest.testGrpcExecutorPool(GrpcServerTest.java:76)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
[ERROR] Failures:
[ERROR] GrpcServerTest.testGrpcExecutorPool:76 expected: <2.0> but was: <0.0>
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
### How was this patch tested?
I run this test more than 1000 times and the test always passes.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #1023: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#discussion_r1267585416
##########
common/src/main/java/org/apache/uniffle/common/rpc/GrpcServer.java:
##########
@@ -77,6 +78,11 @@ protected GrpcServer(
ThreadUtils.getThreadFactory("Grpc"),
grpcMetrics);
}
+
+ //This method is only used for testing
+ public static boolean getExecuted() {
Review Comment:
Because this is boolean type. We would better use `isPoolExecutorHasExecuted` instead of `getExecuted`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #1023: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#discussion_r1267585416
##########
common/src/main/java/org/apache/uniffle/common/rpc/GrpcServer.java:
##########
@@ -77,6 +78,11 @@ protected GrpcServer(
ThreadUtils.getThreadFactory("Grpc"),
grpcMetrics);
}
+
+ //This method is only used for testing
+ public static boolean getExecuted() {
Review Comment:
Because this is boolean type. We would better use `isPoolExecutorHasExecuted` instead of `getExecuted`. Because the test code is in the same package. We don't need to make this method public. Default privilege is enough
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #1023: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#discussion_r1267524768
##########
common/src/main/java/org/apache/uniffle/common/rpc/GrpcServer.java:
##########
@@ -49,6 +49,7 @@ public class GrpcServer implements ServerInterface {
private static final Logger LOG = LoggerFactory.getLogger(GrpcServer.class);
+ public static int testVarCount = 0;
Review Comment:
Could we use private here and add a getter for this variable? Could we use `volatitle` here? Because this variable is used for multi threads? Could we use `boolean` type here? Should we use `static` here? Maybe we should give this variable a better name like `hasExecuted`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #1023: [#388][FOLLOWUP] fix: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#issuecomment-1641530700
Merged to master.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi merged pull request #1023: [#388][FOLLOWUP] fix: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi merged PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #1023: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#discussion_r1267578953
##########
common/src/main/java/org/apache/uniffle/common/rpc/GrpcServer.java:
##########
@@ -77,6 +78,11 @@ protected GrpcServer(
ThreadUtils.getThreadFactory("Grpc"),
grpcMetrics);
}
+
+ //This method is only used for testing
Review Comment:
```suggestion
// This method is only used for testing
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #1023: [#388][FOLLOWUP] fix: Fix the flaky test GrpcServerTest
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#issuecomment-1641490500
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1023?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1023](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1023?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (237d807) into [master](https://app.codecov.io/gh/apache/incubator-uniffle/commit/e5e1b304db19a8b8f1422291b28eaacba7270f4f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e5e1b30) will **increase** coverage by `1.90%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #1023 +/- ##
============================================
+ Coverage 53.67% 55.58% +1.90%
+ Complexity 2521 2091 -430
============================================
Files 382 310 -72
Lines 21646 13909 -7737
Branches 1795 1255 -540
============================================
- Hits 11619 7731 -3888
+ Misses 9323 5754 -3569
+ Partials 704 424 -280
```
| [Impacted Files](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...java/org/apache/uniffle/common/rpc/GrpcServer.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9ycGMvR3JwY1NlcnZlci5qYXZh) | `81.70% <100.00%> (+0.93%)` | :arrow_up: |
... and [75 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1023/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #1023: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#discussion_r1267584533
##########
common/src/main/java/org/apache/uniffle/common/rpc/GrpcServer.java:
##########
@@ -77,6 +78,11 @@ protected GrpcServer(
ThreadUtils.getThreadFactory("Grpc"),
grpcMetrics);
}
+
+ //This method is only used for testing
+ public static boolean getExecuted() {
+ return hasExecuted;
Review Comment:
Think twice. Maybe we should change `hasExecuted` to `poolExecutorHasExecuted`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #1023: [#388][FOLLOWUP] fix: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#issuecomment-1641485340
You run the command to format the files
```
mvn spotless:apply -Pspark3 -Pspark2 -Ptez -Pmr -Phadoop2.8
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #1023: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#issuecomment-1641474746
@zhengchenyu Could you help me review this pr?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #1023: Fix the flaky test GrpcServerTest
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1023:
URL: https://github.com/apache/incubator-uniffle/pull/1023#discussion_r1267524768
##########
common/src/main/java/org/apache/uniffle/common/rpc/GrpcServer.java:
##########
@@ -49,6 +49,7 @@ public class GrpcServer implements ServerInterface {
private static final Logger LOG = LoggerFactory.getLogger(GrpcServer.class);
+ public static int testVarCount = 0;
Review Comment:
Could we use private here and add a getter for this variable?
Could we use `volatitle` here?
Because this variable is used for multi threads?
Could we use `boolean` type here? Should we use `static` here?
Maybe we should give this variable a better name like `hasExecuted`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org