[
https://issues.apache.org/jira/browse/HBASE-20846?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16548792#comment-16548792
]
stack commented on HBASE-20846:
-------------------------------
On the patch...
Non-change in AbstractProcedureScheduler.java ?
Most of patch content is swapping Procedure<?> for Procedure<TEnvironment>.
Trying to understand... now Procedure has a *locked* data member that can be
used by subprocedures. Before subprocedures would supply their own boolean...
sometimes it was named lock... other times locked. A subprocedure that wants a
lock for the life of the procedure returns holdLock as per before? If a
procedure wants to be locked for the procedure step, they do same as before
too? They just call up to the super class to manage the setting an unsetting of
locked?
This lockedWhenLoading is not just for procedures that holdLock for life of the
Procedure run it seems. We'll restore the lock even if we crashed mid-procedure
step on load? That seems right/expected.
920 // persist that we have held the lock. This must be done before
we actually execute the
921 // procedure, otherwise when restarting, we may consider the
procedure does not have a lock,
922 // but it may have already done some changes as we have already
executed it, and if another
923 // procedure gets the lock, then the semantic will be broken if
the holdLock is true, as we do
924 // not expect that another procedure can be executed in the
middle.
925 store.update(this);
Now we do double-store to ProcedureWALStore? Once for the lock and then once
for end-state of the Procedure step? Each time? Slows down pv2 throughput? Any
chance of a reordering so we don't release lock until after we've persisted
step and lock state?
Is this right?
190 * The {@link #doAcquireLock(Object, ProcedureStore)} will be split
into two steps, first, it will
191 * call us to determine whether we need to wait for initialization,
second, it will call
192 * {@link #doAcquireLock(Object, ProcedureStore)} to actually handle
the lock for this procedure.
Seems to say doAcquireLock calls itself.
You remove @InterfaceAudience.Private on methods because class is
@InterfaceAudience.Private? Could remove the Evolving from the class too...
Evolving and Private don't make sense together. Nit.
Nice translation: 94 return
subprocs.stream().mapToLong(Procedure::getProcId).toArray();
These will become annoying?
LOG.debug("{} didn't hold the lock before restarting, skip acquiring
lock.", this);
You probably need them for the moment debugging though? Could leave them in and
then strip them later when known working?
We release the lock even if we didn't have it? And maybe update store.... if
though we didn't have lock.
933 final void doReleaseLock(TEnvironment env, ProcedureStore store) {
934 locked = false;
Will be back with more. Its taking time to digest. Thanks.
This will change how we operate post-master crash but it should make stuff way
better, easier to stand at least, and more predictable.
> 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)
