[GitHub] [spark] HeartSaVioR commented on a change in pull request #24676: [SPARK-27804] LiveListenerBus may create multiple AsyncEventQueues with same name in concurrent scene

[GitBox <gi...@apache.org>]

HeartSaVioR commented on a change in pull request #24676: [SPARK-27804] LiveListenerBus may create multiple AsyncEventQueues with same name in concurrent scene
URL: https://github.com/apache/spark/pull/24676#discussion_r286522772
 
 

 ##########
 File path: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala
 ##########
 @@ -86,44 +87,61 @@ private[spark] class LiveListenerBus(conf: SparkConf) {
   }
 
   /**
-   * Add a listener to a specific queue, creating a new queue if needed. Queues are independent
-   * of each other (each one uses a separate thread for delivering events), allowing slower
-   * listeners to be somewhat isolated from others.
-   */
+    * Add a listener to a specific queue, creating a new queue if needed. Queues are independent
+    * of each other (each one uses a separate thread for delivering events), allowing slower
+    * listeners to be somewhat isolated from others.
+    *
+    * @param listener
+    * @param name name of AsyncEventQueue, @see [[AsyncEventQueue.name]] for more
+    */
   private[spark] def addToQueue(
       listener: SparkListenerInterface,
-      queue: String): Unit = synchronized {
 
 Review comment:
   Here write operation on `queues` is guarded with `synchronized`, so what you describe should not happen as I commented on JIRA issue.
   
   You've asked in JIRA comment why using explicit thread-safe list if the list is guarded. As I'm not the author of code I can't say, but you can trace back to #19211 and read some comments around `queues`.
   
   The list is expected to have very small writing operations, whereas iteration frequently happens. If you see javadoc about `CopyOnWriteArrayList`:
   
   https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/CopyOnWriteArrayList.html
   
   ```
   This is ordinarily too costly, but may be more efficient than alternatives when traversal operations vastly outnumber mutations, and is useful when you cannot or don't want to synchronize traversals, yet need to preclude interference among concurrent threads.
   ```
   
   As you can see `postToQueues`, traversal operations are not guarded with `synchronized`. So the list should be still thread-safe, but it is not to avoid race condition between multiple writes (it's guarded with `synchronized`) but race between read (traversal) and write.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org