[MINOR] refactor: simplify ShuffleWriteClientImpl#genServerToBlocks() #594
Conversation
Codecov Report
@@ Coverage Diff @@
## master #594 +/- ##
============================================
+ Coverage 60.84% 60.85% +0.01%
- Complexity 1797 1800 +3
============================================
Files 214 214
Lines 12398 12387 -11
Branches 1051 1044 -7
============================================
- Hits 7543 7538 -5
+ Misses 4445 4444 -1
+ Partials 410 405 -5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
kaijchen
force-pushed
the
refactor-genServerToBlocks
branch
from
1b6672a
to
ba92962
Compare
kaijchen
changed the title
|
This change is graceful, but i think the performance will be degraded. I don't recommend using lambda in critical path if it brings redundant loop operation. |
Where does it bring redundant loop operation? |
|
Actually we can avoid collecting to list and do it in one pass by reverting 57721a3 |
I just misread. 😂 |
| break; | ||
| } | ||
| Stream<ShuffleServerInfo> servers; | ||
| if (replica > 1 && excludeDefectiveServers) { |
There was a problem hiding this comment.
Change to replica > 1 && excludeDefectiveServers && !defectiveServers.isEmpty()?
There was a problem hiding this comment.
How about excludeDefectiveServers && defectiveServers != null && !defectiveServers.isEmpty()?
Use defectiveServers != null instead of replica > 1 to be consistent with following statements:
| .add(sbi); | ||
| } | ||
| if (excludeServers != null) { | ||
| excludeServers.addAll(selected); |
There was a problem hiding this comment.
How about put this logic after limit of the follow line?
There was a problem hiding this comment.
I mean servers.limit(replicaNum).map((server) -> excludeServers.add(server)).collect(Collectors.toList());
There was a problem hiding this comment.
OK, let's just avoid collect to list.
|
Thanks @xianjingfeng for the review. |
What changes were proposed in this pull request?
Simplify
ShuffleWriteClientImpl#genServerToBlocks().Why are the changes needed?
Simplify code logic.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Covered by
ShuffleWriteClientImplTest#testSendDataWithDefectiveServers().