[#1662] fix(test): Fix Netty related tests #1663

rickyma · 2024-04-19T03:39:32Z

What changes were proposed in this pull request?

Fix some flaky tests.

Why are the changes needed?

Fix: #1662.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unnecessary.

codecov-commenter · 2024-04-19T04:10:17Z

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 54.75%. Comparing base (222f5d4) to head (e13fe8f).
Report is 8 commits behind head on master.

Files	Patch %	Lines
...che/uniffle/client/impl/ShuffleReadClientImpl.java	0.00%	3 Missing and 1 partial ⚠️
...client/impl/grpc/ShuffleServerGrpcNettyClient.java	0.00%	2 Missing ⚠️
...e/shuffle/manager/ShuffleManagerServerFactory.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1663      +/-   ##
============================================
- Coverage     54.82%   54.75%   -0.08%     
- Complexity     2325     2741     +416     
============================================
  Files           366      403      +37     
  Lines         16218    21260    +5042     
  Branches       1482     2019     +537     
============================================
+ Hits           8892    11641    +2749     
- Misses         6804     8880    +2076     
- Partials        522      739     +217

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-04-19T04:25:10Z

Test Results

2 377 files + 77 2 377 suites +77 4h 35m 34s ⏱️ + 34m 16s
920 tests + 1 918 ✅ + 1 1 💤 ±0 0 ❌ ±0 1 🔥 ±0
10 668 runs +173 10 653 ✅ +175 14 💤 ±0 0 ❌ ±0 1 🔥 - 2

For more details on these errors, see this check.

Results for commit 681a3a2. ± Comparison against base commit 45ad0b8.

This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.

org.apache.uniffle.shuffle.manager.ShuffleManagerServerFactoryTest ‑ testShuffleManagerServerType

org.apache.uniffle.shuffle.manager.ShuffleManagerServerFactoryTest ‑ testShuffleManagerServerType{ServerType}[1]
org.apache.uniffle.shuffle.manager.ShuffleManagerServerFactoryTest ‑ testShuffleManagerServerType{ServerType}[2]

♻️ This comment has been updated with latest results.

EnricoMi · 2024-04-19T05:01:50Z

Looks like this PR contains changes that are not related to fixing flaky tests.

EnricoMi · 2024-04-19T05:03:49Z

...common/src/test/java/org/apache/uniffle/shuffle/manager/ShuffleManagerServerFactoryTest.java

-
 public class ShuffleManagerServerFactoryTest {
  @Test
  public void testShuffleManagerServerType() {


Could we use @ParameterizedTest and provide the server types as test parameters?

rickyma · 2024-04-19T05:27:24Z

Looks like this PR contains changes that are not related to fixing flaky tests.

I've removed the docs part of code. I'll put it into another PR.

EnricoMi · 2024-04-19T05:43:49Z

...common/src/test/java/org/apache/uniffle/shuffle/manager/ShuffleManagerServerFactoryTest.java

-  @Test
-  public void testShuffleManagerServerType() {
+  private static Stream<Arguments> shuffleManagerServerTypeProvider() {
+    return Stream.of(Arguments.of(ServerType.GRPC_NETTY), Arguments.of(ServerType.GRPC));


I liked the ServerType.values() that you had earlier, because this automatically tests all types if more are added:

Suggested change

return Stream.of(Arguments.of(ServerType.GRPC_NETTY), Arguments.of(ServerType.GRPC));

return Arrays.stream(ServerType.values());

EnricoMi · 2024-04-19T05:50:44Z

client/src/main/java/org/apache/uniffle/client/impl/ShuffleReadClientImpl.java

      builder.offHeapEnable(offHeapEnabled);
-      builder.clientType(builder.getRssConf().get(RssClientConf.RSS_CLIENT_TYPE));
+      if (builder.getClientType() == null) {
+        builder.clientType(builder.getRssConf().get(RssClientConf.RSS_CLIENT_TYPE));


Why does the builder not take the client type from the rss conf? Is there a use case wher rss conf has one client type but builder.getClientType() has a different type?

This is an issue of historical oversight in the code; it's not written in a standard way. Normally speaking, if there are fields in the builder, which are not null, we should take from the builder first. If the builder doesn't have them, then we should take from RssConf. In the current production code, even if the builder has the field(which is not null), we still basically take it from RssConf.

And that is the reason why https://github.com/apache/incubator-uniffle/pull/1663/files#diff-2c63c456bb33c64e05cbb946b316f6b26ac9153fc8ef112024e47bf8c3fc3c5aR46 does not fail when we set rss.client.type to GRPC_NETTY. Because when calling the builder's build() method, the client type will be reset to the default value GRPC through RssConf, so there has actually been a hidden bug here all along.

Of course, it's not impossible to remove those fields from the builder and set them all through RssConf, it's just harder to maintain. I think, since we already have those fields in the builder, we should support setting them through the builder, otherwise it's better to remove them altogether.

EnricoMi · 2024-04-22T15:24:34Z

...ark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerServerFactory.java

  public GrpcServer getServer(ShuffleManagerGrpcService service) {
    ServerType type = conf.get(RssBaseConf.RPC_SERVER_TYPE);
-    if (type == ServerType.GRPC) {
+    if (type == ServerType.GRPC || type == ServerType.GRPC_NETTY) {


This does not look like fixing flaky tests but like a feature + testing it. Maybe the PR title should be rephrased.

EnricoMi

LGTM!

zuston · 2024-04-23T11:10:31Z

Could you help merge this? @EnricoMi

### What changes were proposed in this pull request? Fix some flaky tests. ### Why are the changes needed? Fix: #1662. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unnecessary.

rickyma added 7 commits April 17, 2024 13:12

[apache#1651] improvement(netty): Set Netty as the default server type

4d04254

[apache#1651] improvement(netty): Set Netty as the default server type

84dda59

Fix integration test

cfb02b9

Fix integration test

043e787

Fix integration test

a3fe371

Fix integration test

f6bb50c

Set nettyPort to -1 when Netty is not enabled

545ad8c

rickyma force-pushed the issue-1662 branch 3 times, most recently from 0f721dc to 3f5d5da Compare April 19, 2024 03:53

[apache#1662] fix(test): Fix flaky tests

e13fe8f

rickyma force-pushed the issue-1662 branch from 3f5d5da to e13fe8f Compare April 19, 2024 04:00

EnricoMi reviewed Apr 19, 2024

View reviewed changes

rickyma added 2 commits April 19, 2024 13:16

Fix reviews

97f5bbd

Fix reviews

eb45ff2

rickyma changed the title ~~[#1662] fix(test): Fix flaky tests and update docs~~ [#1662] fix(test): Fix flaky tests Apr 19, 2024

rickyma requested a review from EnricoMi April 19, 2024 05:27

EnricoMi reviewed Apr 19, 2024

View reviewed changes

Fix reviews

681a3a2

rickyma requested a review from EnricoMi April 20, 2024 06:12

EnricoMi reviewed Apr 22, 2024

View reviewed changes

EnricoMi approved these changes Apr 22, 2024

View reviewed changes

rickyma changed the title ~~[#1662] fix(test): Fix flaky tests~~ [#1662] fix(test): Fix Netty related tests Apr 23, 2024

zuston approved these changes Apr 23, 2024

View reviewed changes

zuston merged commit 3bc0ee5 into apache:master Apr 25, 2024

rickyma deleted the issue-1662 branch May 5, 2024 08:33

	return Stream.of(Arguments.of(ServerType.GRPC_NETTY), Arguments.of(ServerType.GRPC));
	return Arrays.stream(ServerType.values());

[#1662] fix(test): Fix Netty related tests #1663

[#1662] fix(test): Fix Netty related tests #1663

Uh oh!

Conversation

rickyma commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

codecov-commenter commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

EnricoMi commented Apr 19, 2024

Uh oh!

EnricoMi Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

rickyma Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

rickyma commented Apr 19, 2024

Uh oh!

EnricoMi Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

rickyma Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

EnricoMi Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

rickyma Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EnricoMi Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

EnricoMi left a comment

Choose a reason for hiding this comment

Uh oh!

zuston commented Apr 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rickyma commented Apr 19, 2024 •

edited

Loading

codecov-commenter commented Apr 19, 2024 •

edited

Loading

github-actions bot commented Apr 19, 2024 •

edited

Loading

rickyma Apr 19, 2024 •

edited

Loading