Github user zhichao-li commented on the pull request:
https://github.com/apache/spark/pull/5508#issuecomment-92773590
>These are slightly different ideas. I agree in practice here that there is
no visibility problem that volatile would solve that isn't already taken care
of by the memory barrier from synchronized. However I also don't imagine this
would cause any measurable overhead from the extra read. Have you measured some
improvement? If not, I am not sure this is worth changing since the moment this
field becomes read outside synchronized blocks it's technically unsafe again.
Just thinking of it would be a good practice to avoid the usage of
`volatile` and `synchronized` together
Each small batch currentBuffer would be r/w and it's private too, so even
if it's only a tiny overhead it would be much if they are sum up
together(haven't done benchmark regarding this though).
This field is a mutable array, so even if it's modified by `volatile`, I
guess technically it's not safe to use it in the other place without locking.
I'm fine to stay the same, given that we always set the batch interval to
be no less then 500ms and also there would be no measurable improvement
compared with that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
