WIP #873 Remove Sync and added StripedLock#1710
WIP #873 Remove Sync and added StripedLock#1710tynyttie wants to merge 2 commits intoapache:mainfrom
Conversation
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
It looks like the Guava Striped lock is marked as @Beta, which means that it is risk for us to use without potentially introducing dependency problems with different versions of Guava that users could end up having on their class path. This is less of a risk with Hadoop 3 and server-side code, but it's still something we probably want to avoid. It might not be a good idea to use Striped from Guava at this time, especially if you're not seeing performance improvements for doing so (though, it's not clear how you're testing for that).
| import java.util.concurrent.atomic.AtomicInteger; | ||
| import java.util.concurrent.locks.Lock; | ||
|
|
||
| import com.google.common.util.concurrent.Striped; |
There was a problem hiding this comment.
Just as a note, it appears it has been in beta since 2012, v 13.0 (now v 30.0 is released). I don't expect it to leave beta anytime soon.
There was a problem hiding this comment.
It doesn't seem that long ago since we were using version 11 of Guava, because that's what Hadoop was using. Guava moves fast. The beta methods can change quickly and can quickly become incompatible with other libraries that require specific versions of Guava. I still think this is unsafe.
More importantly, if there's no observable performance benefit, it's not worth the risk.
There was a problem hiding this comment.
I would avoid Guava striped locks because its beta. I have used this pattern w/o Guava using an array of locks like :
There was a problem hiding this comment.
Use of Beta can be dangerous as we have been previously burned by it. I did some work to remove uses of Beta code in Accumulo. See https://issues.apache.org/jira/browse/ACCUMULO-4702
I would listen to this important declaration:
If your code is a library itself (i.e. it is used on the CLASSPATH of users outside your own control), you should not use beta API
|
@tynyttie what type of test did you run? Was there concurrent bulk import operations running? |
|
Closing outdated PR due to comment on original ticket: #873 (comment) |
5 participants
When tested, I did not notice any performance benefits.