advancedxy commented on code in PR #428:
URL: https://github.com/apache/incubator-uniffle/pull/428#discussion_r1055428805
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -579,4 +602,11 @@ void removeShuffleDataSync(String appId, int shuffleId) {
public ShuffleDataDistributionType getDataDistributionType(String appId) {
return shuffleTaskInfos.get(appId).getDataDistType();
}
+
+ private void triggerFlush() {
+ synchronized (this.shuffleBufferManager) {
Review Comment:
> Sorry, I can't get this point of this change
Based on the context, you mean why the `synchronized` lock right?
The lock is hold the same reason as `ShuffleBufferManager` locks it to flush
data, the exact code you referenced.
I believe it is used to avoid data race.
In current codebase, the `triggerFlush` is only called per
`rss.server.shuffleBufferManager.trigger.flush.interval`, which should be 2-5
mins on server side. That shouldn't be too much overhead?
As for why `triggerFlush` is introduced in this PR, its main reason is to
reduce UT changes for the behavior changes as `preAllocatedSize` is decreased
in Grpc layer, rather than the cache data layer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
