Re: [PR] [CELEBORN-1152] fix GetShuffleId RPC NPE for empty shuffle [incubator-celeborn]

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

waitinfuture commented on code in PR #2136:
URL: https://github.com/apache/incubator-celeborn/pull/2136#discussion_r1421260931


##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -653,16 +654,31 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
       appShuffleId: Int,
       appShuffleIdentifier: String,
       isWriter: Boolean): Unit = {
-    val shuffleIds = shuffleIdMapping.computeIfAbsent(
-      appShuffleId,
-      new function.Function[Int, scala.collection.mutable.LinkedHashMap[String, (Int, Boolean)]]() {
-        override def apply(id: Int)
-            : scala.collection.mutable.LinkedHashMap[String, (Int, Boolean)] = {
-          val newShuffleId = shuffleIdGenerator.getAndIncrement()
-          logInfo(s"generate new shuffleId $newShuffleId for appShuffleId $appShuffleId appShuffleIdentifier $appShuffleIdentifier")
-          scala.collection.mutable.LinkedHashMap(appShuffleIdentifier -> (newShuffleId, true))
-        }
-      })
+    val shuffleIds =
+      if (isWriter) {
+        shuffleIdMapping.computeIfAbsent(
+          appShuffleId,
+          new function.Function[
+            Int,
+            scala.collection.mutable.LinkedHashMap[String, (Int, Boolean)]]() {
+            override def apply(id: Int)
+                : scala.collection.mutable.LinkedHashMap[String, (Int, Boolean)] = {
+              val newShuffleId = shuffleIdGenerator.getAndIncrement()
+              logInfo(s"generate new shuffleId $newShuffleId for appShuffleId $appShuffleId appShuffleIdentifier $appShuffleIdentifier")
+              scala.collection.mutable.LinkedHashMap(appShuffleIdentifier -> (newShuffleId, true))
+            }
+          })
+      } else {
+        shuffleIdMapping.get(appShuffleId)
+      }
+
+    if (shuffleIds == null) {
+      logWarning(s"unknown appShuffleId $appShuffleId, maybe no shuffle data for this shuffle")
+      val pbGetShuffleIdResponse =
+        PbGetShuffleIdResponse.newBuilder().setShuffleId(UNKNOWN_APP_SHUFFLE_ID).build()

Review Comment:
   We can short-circuit  `ShuffleClientImpl#readPartition` if partitionId is -1 and return `CelebornInputStream.empty()` without calling `loadFileGroup(shuffleId, partitionId)`



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org