The changes in pom.xml are not needed for this fix, right ?
That's right, sorry about that. Just helps get m2eclipse to compile everything correctly.
We need change all the CP hooks signatures? CPs will(should) be able to acquire locks and release?
The CP changes required would be in RegionObserver:
- preBatchMutate(...,MiniBatchOperationInProgress<Pair<Mutation, Integer>> miniBatchOp) becomes
- preBatchMutate(...,MiniBatchOperationInProgress<Mutation> miniBatchOp)
- postBatchMutate(...,MiniBatchOperationInProgress<Pair<Mutation, Integer>> miniBatchOp) becomes
- postBatchMutate(...,MiniBatchOperationInProgress<Mutation> miniBatchOp)
I don't have much experience with coprocessor usage, but it seems unlikely to me someone would require manipulating the set of row locks in the middle of a miniBatch mutation. Would it be prudent to poll the mailing list first?
Re: patch v5
Looks good to me. Anoop's notion of favoring the case where the lock is new versus the lock is already owned is an interesting one. Two possibilities of code with two cases:
Code A: patch v5 - first check rowsAlreadyLocked then getLock
- Case 1 (lock is new): hash comparison in rowsAlreadyLocked likely returns !contains quickly. getLock likely also quickly finds no hash match in concurrent hashmap and inserts.
- Case 2 (lock is already owned): hashCode match then bytewise equals comparison in rowsAlreadyLocked.contains. getLock not called
Code B: first try getLock, then if unable to getLock check rowsAlreadyLocked
- Case 1 (lock is new): getLock quickly finds no hash match in concurrent hashmap and inserts.
- Case 2 (lock is already owned): getLock checks concurrent hashmap finds hashCode match then does bytewise equals comparison. then rowsAlreadyLocked also gets hashCode match then bytewise equals comparison
So Code A has an extra hash check for Case 1 and Code B has an extra hash check and bytewise equal compare for Case 2. I'd favor current patch (Code A) as it's a smaller and constant time addition and I think the code is a bit more intuitive, but like the property in Code B that it has no impact on Case 1 compared to the existing 0.94 releases. I'd be content if Rahul's tests on the latest patch show imapct is small.
Is it ok if we sort it out on the client side itself?
Not for 0.94 as it would be changing the contract on existing clients which may suddenly fail to respect row locks if they aren't updated. Though it would be interesting to ask clients to sort for performance gains and then re-sort with a stable sort on the server. But if people are happy with the other approaches discussed, they sound good to me.
Should we commit something like the 0.94 patch to trunk as well, for now; and then regroup and add re-entrant locks in a different jira?
+1 so long as people are comfortable with performance. Let's give Rahul a chance to run his perf test on this patch if no one's in a hurry.
I will try to update my reentrant locks patch for trunk. After posting it up I was considering introducing that lock groups idea too and using it for checkAndPut. May play with the code there unless someone beats me to it.
Thanks to everyone for the attention on this issue!