You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spark.apache.org by Oleksandr Vayda <ol...@gmail.com> on 2017/06/26 09:27:06 UTC
SQLListener concurrency bug?
Hi all,
Reading the source code of the org.apache.spark.sql.execution.ui.SQLListener,
specifically this place -
https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLListener.scala#L328
def getFailedExecutions: Seq[SQLExecutionUIData] = synchronized {
failedExecutions
}
def getCompletedExecutions: Seq[SQLExecutionUIData] = synchronized {
completedExecutions
}
I believe the synchronized block is used here incorrectly. If I get it
right the main purpose here is to synchronize access to the mutable
collections from the UI (read) and the event bus (read/write) threads. But
in the current implementation the "synchronized" blocks return bare
references to mutable collections and in fact nothing gets synchronized.
Is it a bug?
Sincerely yours,
Oleksandr Vayda
mobile: +420 604 113 056
Re: SQLListener concurrency bug?
Posted by Oleksandr Vayda <ol...@gmail.com>.
Oh, sorry, I was wrong. Concurrent collections in Scala are available since
2.8. Any objections against replacing mutable list and synchronized with a
concurrent collection like or based on TrieMap for instance?
On Wed, Jun 28, 2017, 16:05 Oleksandr Vayda <ol...@gmail.com>
wrote:
> Cool. I will be happy to create a PR. The simplest and most obvious
> solution that came to my mind was using Java concurrent collections instead
> of Scala mutable. Don't you mind to have this bit of Java inside Spark? :)
> Or perhaps we could use Scala concurrent collections, but they are only
> available in Scala 2.12 AFAIK.
>
> Alex
>
> On Tue, Jun 27, 2017, 05:42 Shixiong(Ryan) Zhu <sh...@databricks.com>
> wrote:
>
>> Right now they are safe because the caller also calls synchronized when
>> using them. This is to avoid copying objects. It's probably a bad design.
>> If you want to refactor them, PR is welcome.
>>
>> On Mon, Jun 26, 2017 at 2:27 AM, Oleksandr Vayda <
>> oleksandr.vayda@gmail.com> wrote:
>>
>>> Hi all,
>>>
>>> Reading the source code of the org.apache.spark.sql.execution.ui.
>>> SQLListener, specifically this place -
>>> https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLListener.scala#L328
>>>
>>> def getFailedExecutions: Seq[SQLExecutionUIData] = synchronized {
>>> failedExecutions
>>> }
>>> def getCompletedExecutions: Seq[SQLExecutionUIData] = synchronized {
>>> completedExecutions
>>> }
>>> I believe the synchronized block is used here incorrectly. If I get it
>>> right the main purpose here is to synchronize access to the mutable
>>> collections from the UI (read) and the event bus (read/write) threads. But
>>> in the current implementation the "synchronized" blocks return bare
>>> references to mutable collections and in fact nothing gets synchronized.
>>> Is it a bug?
>>>
>>> Sincerely yours,
>>> Oleksandr Vayda
>>>
>>> mobile: +420 604 113 056 <+420%20604%20113%20056>
>>>
>>
>>
Re: SQLListener concurrency bug?
Posted by Oleksandr Vayda <ol...@gmail.com>.
Cool. I will be happy to create a PR. The simplest and most obvious
solution that came to my mind was using Java concurrent collections instead
of Scala mutable. Don't you mind to have this bit of Java inside Spark? :)
Or perhaps we could use Scala concurrent collections, but they are only
available in Scala 2.12 AFAIK.
Alex
On Tue, Jun 27, 2017, 05:42 Shixiong(Ryan) Zhu <sh...@databricks.com>
wrote:
> Right now they are safe because the caller also calls synchronized when
> using them. This is to avoid copying objects. It's probably a bad design.
> If you want to refactor them, PR is welcome.
>
> On Mon, Jun 26, 2017 at 2:27 AM, Oleksandr Vayda <
> oleksandr.vayda@gmail.com> wrote:
>
>> Hi all,
>>
>> Reading the source code of the org.apache.spark.sql.execution.ui.
>> SQLListener, specifically this place -
>> https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLListener.scala#L328
>>
>> def getFailedExecutions: Seq[SQLExecutionUIData] = synchronized {
>> failedExecutions
>> }
>> def getCompletedExecutions: Seq[SQLExecutionUIData] = synchronized {
>> completedExecutions
>> }
>> I believe the synchronized block is used here incorrectly. If I get it
>> right the main purpose here is to synchronize access to the mutable
>> collections from the UI (read) and the event bus (read/write) threads. But
>> in the current implementation the "synchronized" blocks return bare
>> references to mutable collections and in fact nothing gets synchronized.
>> Is it a bug?
>>
>> Sincerely yours,
>> Oleksandr Vayda
>>
>> mobile: +420 604 113 056 <+420%20604%20113%20056>
>>
>
>
Re: SQLListener concurrency bug?
Posted by "Shixiong(Ryan) Zhu" <sh...@databricks.com>.
Right now they are safe because the caller also calls synchronized when
using them. This is to avoid copying objects. It's probably a bad design.
If you want to refactor them, PR is welcome.
On Mon, Jun 26, 2017 at 2:27 AM, Oleksandr Vayda <ol...@gmail.com>
wrote:
> Hi all,
>
> Reading the source code of the org.apache.spark.sql.execution.ui.
> SQLListener, specifically this place - https://github.com/apache/
> spark/blob/master/sql/core/src/main/scala/org/apache/
> spark/sql/execution/ui/SQLListener.scala#L328
>
> def getFailedExecutions: Seq[SQLExecutionUIData] = synchronized {
> failedExecutions
> }
> def getCompletedExecutions: Seq[SQLExecutionUIData] = synchronized {
> completedExecutions
> }
> I believe the synchronized block is used here incorrectly. If I get it
> right the main purpose here is to synchronize access to the mutable
> collections from the UI (read) and the event bus (read/write) threads. But
> in the current implementation the "synchronized" blocks return bare
> references to mutable collections and in fact nothing gets synchronized.
> Is it a bug?
>
> Sincerely yours,
> Oleksandr Vayda
>
> mobile: +420 604 113 056 <+420%20604%20113%20056>
>