[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #621: [#378] feat: introduce storage manager selector

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

advancedxy commented on code in PR #621:
URL: https://github.com/apache/incubator-uniffle/pull/621#discussion_r1109799508


##########
server/src/main/java/org/apache/uniffle/server/storage/multi/DefaultStorageManagerSelector.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.uniffle.server.storage.multi;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.server.ShuffleDataFlushEvent;
+import org.apache.uniffle.server.storage.AbstractStorageManagerFallbackStrategy;
+import org.apache.uniffle.server.storage.StorageManager;
+
+import static org.apache.uniffle.server.ShuffleServerConf.FLUSH_COLD_STORAGE_THRESHOLD_SIZE;
+import static org.apache.uniffle.server.ShuffleServerConf.MULTISTORAGE_SELECTOR_COLD_STORAGE_PREFER;
+
+public class DefaultStorageManagerSelector implements StorageManagerSelector {
+  private final StorageManager warmStorageManager;
+  private final StorageManager coldStorageManager;
+  private final AbstractStorageManagerFallbackStrategy fallbackStrategy;
+  private final ColdStoragePreferredFactor preferColdStorageFactor;
+  private final long flushColdStorageThresholdSize;
+
+  public DefaultStorageManagerSelector(
+      StorageManager warmStorageManager,
+      StorageManager coldStorageManager,
+      AbstractStorageManagerFallbackStrategy fallbackStrategy,
+      RssConf rssConf) {
+    this.coldStorageManager = coldStorageManager;
+    this.warmStorageManager = warmStorageManager;
+    this.fallbackStrategy = fallbackStrategy;
+    this.flushColdStorageThresholdSize = rssConf.get(FLUSH_COLD_STORAGE_THRESHOLD_SIZE);
+    this.preferColdStorageFactor = rssConf.get(MULTISTORAGE_SELECTOR_COLD_STORAGE_PREFER);

Review Comment:
   I believe you are coupling the default storage manager selector with a specialized storage manager selector.
   
   I would make the default storage manager logic simple and align with original implementation.
   
   For flush huge-partition only event to coldStorage, you could impl a new storage manager selector, which could be configured via options.



##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -671,7 +671,7 @@ public ShuffleTaskInfo getShuffleTaskInfo(String appId) {
 
   private void triggerFlush() {
     synchronized (this.shuffleBufferManager) {
-      this.shuffleBufferManager.flushIfNecessary();
+      this.shuffleBufferManager.flushIfNecessary(this::getPartitionDataSize);

Review Comment:
   I can get you intention.. But I believe passing the `getPartitionDataSize` method ref around is adding a lot of interface change and it's adding maintenance overhead.
   
   How about make ShuffleTaskManager a private field of ShuffleBufferManager? Then all this interface change is unnecessary.



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