[
https://issues.apache.org/jira/browse/HBASE-20846?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16549096#comment-16549096
]
Duo Zhang commented on HBASE-20846:
-----------------------------------
{quote}
Most of patch content is swapping Procedure<?> for Procedure<TEnvironment>.
{quote}
Just want to eliminate the warnings. Will upload the patch to RB when the pre
commit result is OK, but the jenkins is in a bad state... The infra team said
that they will move the jenkins server to a new machine at Saturday, will try
later.
The changes of this patch are:
1. Make hasLock method final, and add a locked field in Procedure to record
whether we have the lock. We will set it to true in doAcquireLock and to false
in doReleaseLock. The sub procedures do not need to manage it any more.
2. Also added a locked field in the proto message. When storing, the field will
be set according to the return value of hasLock. And when loading, there is a
new field in Procedure called lockedWhenLoading. We will set it to true if the
locked field in proto message is true.
3. The reason why we can not set the locked field directly to true by calling
doAcquireLock is that, during initialization, most procedures need to wait
until master is initialized. So the solution here is that, we introduced a new
method called waitInitialized in Procedure, and move the wait master
initialized related code from acquireLock to this method. And we added a
restoreLock method to Procedure, if lockedWhenLoading is true, we will call the
acquireLock to get the lock, but do not set locked to true. And later when we
call doAcquireLock and pass the waitInitialized check, we will test
lockedWhenLoading, if it is true, when we just set the locked field to true and
return, without actually calling the acquireLock method since we have already
called it once.
{quote}
We release the lock even if we didn't have it? And maybe update store.... if
though we didn't have lock.
{quote}
No, we will test hasLock before calling doReleaseLock. The only place where we
do not test hasLock is in execProcedure, where we can make sure that we have
the lock.
And I think there are some space for optimization, for example, if holdLock is
false when maybe we do not need to store the lock operation, but as I said
above, later we want to make holdLock depend on the state, for example, for
ModifyTableProcedure, all the states before the last state which we reopen all
the regions, the holdLock should be true as we do not want other one to jump in
the middle, but after the last state where we schedule a lot of sub procedures
to reopen all the regions, we need to release the lock, so holdLock should be
false. So I'm not sure if it is safe to use holdLock to determine whether we
should store the log operations. May be OK, but need to think more.
And also, for release lock, I think it could be merged with the
updateStoreOnExec call, but the logic is a bit complicated as the
updateStoreOnExec is in a loop, so in this patch I plan to implement it in a
simple way. We can optimize it later.
> Restore procedure locks when master restarts
> --------------------------------------------
>
> Key: HBASE-20846
> URL: https://issues.apache.org/jira/browse/HBASE-20846
> Project: HBase
> Issue Type: Sub-task
> Affects Versions: 2.1.0
> Reporter: Allan Yang
> Assignee: Duo Zhang
> Priority: Major
> Fix For: 3.0.0, 2.0.2, 2.1.1
>
> Attachments: HBASE-20846-v1.patch, HBASE-20846-v2.patch,
> HBASE-20846-v3.patch, HBASE-20846-v4.patch, HBASE-20846-v4.patch,
> HBASE-20846.branch-2.0.002.patch, HBASE-20846.branch-2.0.patch,
> HBASE-20846.patch
>
>
> Found this one when investigating ModifyTableProcedure got stuck while there
> was a MoveRegionProcedure going on after master restart.
> Though this issue can be solved by HBASE-20752. But I discovered something
> else.
> Before a MoveRegionProcedure can execute, it will hold the table's shared
> lock. so,, when a UnassignProcedure was spwaned, it will not check the
> table's shared lock since it is sure that its parent(MoveRegionProcedure) has
> aquired the table's lock.
> {code:java}
> // If there is parent procedure, it would have already taken xlock, so no
> need to take
> // shared lock here. Otherwise, take shared lock.
> if (!procedure.hasParent()
> && waitTableQueueSharedLock(procedure, table) == null) {
> return true;
> }
> {code}
> But, it is not the case when Master was restarted. The child
> procedure(UnassignProcedure) will be executed first after restart. Though it
> has a parent(MoveRegionProcedure), but apprently the parent didn't hold the
> table's lock.
> So, since it began to execute without hold the table's shared lock. A
> ModifyTableProcedure can aquire the table's exclusive lock and execute at the
> same time. Which is not possible if the master was not restarted.
> This will cause a stuck before HBASE-20752. But since HBASE-20752 has fixed,
> I wrote a simple UT to repo this case.
> I think we don't have to check the parent for table's shared lock. It is a
> shared lock, right? I think we can acquire it every time we need it.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
