-
Notifications
You must be signed in to change notification settings - Fork 168
[#1662] fix(test): Fix Netty related tests #1663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4d04254
84dda59
cfb02b9
043e787
a3fe371
f6bb50c
545ad8c
e13fe8f
97f5bbd
eb45ff2
681a3a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,12 +102,13 @@ public ShuffleReadClientImpl(ShuffleClientFactory.ReadClientBuilder builder) { | |
| readBufferSize = Integer.MAX_VALUE; | ||
| } | ||
| boolean offHeapEnabled = builder.getRssConf().get(RssClientConf.OFF_HEAP_MEMORY_ENABLE); | ||
|
|
||
| builder.indexReadLimit(indexReadLimit); | ||
| builder.storageType(storageType); | ||
| builder.readBufferSize(readBufferSize); | ||
| 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)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
| } | ||
| } else { | ||
| // most for test | ||
| RssConf rssConf = (builder.getRssConf() == null) ? new RssConf() : builder.getRssConf(); | ||
|
|
@@ -131,7 +132,9 @@ public ShuffleReadClientImpl(ShuffleClientFactory.ReadClientBuilder builder) { | |
| builder.rssConf(rssConf); | ||
| builder.offHeapEnable(false); | ||
| builder.expectedTaskIdsBitmapFilterEnable(false); | ||
| builder.clientType(rssConf.get(RssClientConf.RSS_CLIENT_TYPE)); | ||
| if (builder.getClientType() == null) { | ||
| builder.clientType(rssConf.get(RssClientConf.RSS_CLIENT_TYPE)); | ||
| } | ||
| } | ||
| if (builder.getIdHelper() == null) { | ||
| builder.idHelper(new DefaultIdHelper(BlockIdLayout.from(builder.getRssConf()))); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look like fixing flaky tests but like a feature + testing it. Maybe the PR title should be rephrased.