[jira] [Commented] (HBASE-20975) Lock may not be taken while rolling back procedure

["Duo Zhang (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HBASE-20975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16561609#comment-16561609 ] 

Duo Zhang commented on HBASE-20975:
-----------------------------------

+1 on removing it for now. We have optimized too much before getting things correct... Let's keep the logic simple first.

Also, there are some bad style issues.

{code}
while (stackTail --> 0) {
{code}

At least let's remove the space between 'stackTail' and '--', it looks like '-->' is the operator...

And do not do assignment in the condition block of if. Let's change to

{code}
LockState lockState = acquireLock(proc);
if (lockState != LockState.LOCK_ACQUIRED)
{code}

> Lock may not be taken while rolling back procedure
> --------------------------------------------------
>
>                 Key: HBASE-20975
>                 URL: https://issues.apache.org/jira/browse/HBASE-20975
>             Project: HBase
>          Issue Type: Sub-task
>          Components: amv2
>    Affects Versions: 2.1.0, 2.0.1
>            Reporter: Allan Yang
>            Assignee: Allan Yang
>            Priority: Major
>         Attachments: HBASE-20975.branch-2.0.001.patch
>
>
> Find this one when investigating HBASE-20921, too.
> Here is some code from executeRollback in ProcedureExecutor.java.
> {code}
>     boolean reuseLock = false;
>     while (stackTail --> 0) {
>       final Procedure proc = subprocStack.get(stackTail);
>       LockState lockState;
>       //If reuseLock, then don't acquire the lock
>       if (!reuseLock && (lockState = acquireLock(proc)) != LockState.LOCK_ACQUIRED) {
>         return lockState;
>       }
>       lockState = executeRollback(proc);
>       boolean abortRollback = lockState != LockState.LOCK_ACQUIRED;
>       abortRollback |= !isRunning() || !store.isRunning();
>       //If the next procedure in the stack is the current one, then reuseLock = true
>       reuseLock = stackTail > 0 && (subprocStack.get(stackTail - 1) == proc) && !abortRollback;
>       //If reuseLock, don't releaseLock
>       if (!reuseLock) {
>         releaseLock(proc, false);
>       }
>       if (abortRollback) {
>         return lockState;
>       }
>       subprocStack.remove(stackTail);
>       if (proc.isYieldAfterExecutionStep(getEnvironment())) {
>         return LockState.LOCK_YIELD_WAIT;
>       }
>       //But, here, lock is released no matter reuseLock is true or false
>       if (proc != rootProc) {
>         execCompletionCleanup(proc);
>       }
>     }
> {code}
> You can see my comments in the code above, reuseLock can cause the procedure executing(rollback) without a lock. Though I haven't found any bugs introduced by this issue, it is indeed a potential bug need to fix.
> I think we can just remove the reuseLock logic. Acquire and release lock every time. 



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)