Github user tdas commented on the pull request:
https://github.com/apache/spark/pull/2991#issuecomment-62976670
@jerryshao Here is another round of changes from me.
You correctly identified a flaw in the lock logic in the last change I
made. I played around with different implementations, and I came up with two
implementation that I think are correct, and tries to preserve the
BlockGenerator performance for existing receivers. Both of them are pulls
requests on your repo.
1. Refactor 1, https://github.com/jerryshao/apache-spark/pull/7: Here I
attempt to fix the flaw by taking receiver lock before updating the buffer. And
this is done by extending the BlockGenerator and overriding the
updateCurrentBuffer method to take receiver lock first. This ensures deadlock
free locking by always taking locks in the same order - receiver lock followed
by block generator lock. The default block generator code path is not affected,
so other receiver should not be affected either.
2. Refactor2, https://github.com/jerryshao/apache-spark/pull/6: I
essentially reverted back to your original proposal. :) As I tried out all
possible implementations, and finally got "Refactor 1", I realized that it was
more complicated than what you proposed. So I reverted back to that, and added
a lot of scala docs explaining the behavior. Personally I am in favor of this
now.
Besides I also eliminated more duplicate code form the unit tests and
merged two unit tests to reduce test run times. Please take a look.
---
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
