Remove lock from URManager’s state only if lock znode deletion has succeeded#2206
Remove lock from URManager’s state only if lock znode deletion has succeeded#2206eolivelli merged 3 commits intoapache:masterfrom
Conversation
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
How much this patch is critical?
Should be roll a 4.10.1 hotfix release?
|
How much this patch is critical? This is not critical fix. Probability of happening is low, we internally configured number of zkOperations retry count to 3, but where as in the community version, it is set to infinite number. |
|
HI @reddycharan is the |
|
@Ghatage it is not dupe. I'm creating new class to mimic (mock/inject) certain behavior needed for this particular testcase. Scope of this Mock class is local to this testsuite. So it is not meant to be merged. |
|
Yes, we can have two classes, they are doing different things. |
| /* | ||
| * Start RW. | ||
| */ | ||
| ReplicationWorker rw = new ReplicationWorker(baseConf, bkWithMockZK, false, NullStatsLogger.INSTANCE); |
There was a problem hiding this comment.
Do we need to shutdown this object and the other ones in a finallyblock in order to teardown properly the test?
|
@reddycharan do you have time to move forward this work? |
…cceeded. (apache#462) - in ZkLedgerUnderreplicationManager.releaseUnderreplicatedLedger remove ‘lock’ from ‘heldLocks’ only if lock znode deletion has succeeded. - This is needed because, if RW.logBKExceptionAndReleaseLedger fails to delete the lock znode, then it needs to be tried once more in the finally block of 'rereplicate(long ledgerIdToReplicate)’ before giving up and shutting down RW.
reddycharan
force-pushed
the
fixzklossissue
branch
from
39c0de2 to
20e417e
Compare
|
hey @eolivelli can you approve this PR. Also, btw, can I use this "Squash and Merge" option for merging code to master? |
|
We are using the script not this button. Thank you very much! |
|
I can't merge it now because I don't have my laptop. I will do it as soon as possible |
…cceeded Descriptions of the changes in this PR: - in ZkLedgerUnderreplicationManager.releaseUnderreplicatedLedger remove ‘lock’ from ‘heldLocks’ only if lock znode deletion has succeeded. - This is needed because, if RW.logBKExceptionAndReleaseLedger fails to delete the lock znode, then it needs to be tried once more in the finally block of 'rereplicate(long ledgerIdToReplicate)’ before giving up and shutting down RW. Reviewers: Enrico Olivelli <eolivelli@gmail.com> This closes apache#2206 from reddycharan/fixzklossissue
…cceeded Descriptions of the changes in this PR: - in ZkLedgerUnderreplicationManager.releaseUnderreplicatedLedger remove ‘lock’ from ‘heldLocks’ only if lock znode deletion has succeeded. - This is needed because, if RW.logBKExceptionAndReleaseLedger fails to delete the lock znode, then it needs to be tried once more in the finally block of 'rereplicate(long ledgerIdToReplicate)’ before giving up and shutting down RW. Reviewers: Enrico Olivelli <eolivelli@gmail.com> This closes apache#2206 from reddycharan/fixzklossissue
3 participants
Descriptions of the changes in this PR:
from ‘heldLocks’ only if lock znode deletion has succeeded.
the lock znode, then it needs to be tried once more in the finally block of
'rereplicate(long ledgerIdToReplicate)’ before giving up and shutting down RW.