[GitHub] [incubator-uniffle] leixm commented on a diff in pull request #775: [#757] Separate flush thread pools for different storage type.

["leixm (via GitHub)" <gi...@apache.org>]

leixm commented on code in PR #775:
URL: https://github.com/apache/incubator-uniffle/pull/775#discussion_r1151330034


##########
server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java:
##########
@@ -118,14 +123,24 @@ private void startEventProcessor() {
     processEventThread.start();
   }
 
-  protected Executor createFlushEventExecutor() {
+  protected void initFlushEventExecutor() {

Review Comment:
   Ok.



##########
server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java:
##########
@@ -145,7 +160,14 @@ protected void eventLoop() {
   protected void processNextEvent() {
     try {
       ShuffleDataFlushEvent event = flushQueue.take();
-      threadPoolExecutor.execute(() -> processEvent(event));
+      Storage storage = storageManager.selectStorage(event);

Review Comment:
   > It looks really strange for selecting storage here, which will select again before doing flush.
   > 
   > And the selected storage will be changed if writing failed. I think we'd better refactor flushing part.
   
   You're right.



-- 
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