Apache9 commented on code in PR #5816:
URL: https://github.com/apache/hbase/pull/5816#discussion_r1581817417
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ServerRemoteProcedure.java:
##########
@@ -137,6 +140,10 @@ synchronized void remoteOperationDone(MasterProcedureEnv
env, Throwable error) {
getProcId());
return;
}
+ //below persistence is added so that if report goes to last active master,
it throws exception
+ state =
MasterProcedureProtos.ServerRemoteProcedureState.SERVER_REMOTE_PROCEDURE_REPORT_SUCCEED;
+
env.getMasterServices().getMasterProcedureExecutor().getStore().update(this);
+
complete(env, error);
Review Comment:
I do not think you need to consider a master with old code become active, as
we are trying to fix a bug in the old code...
And I still do not get your point on why you must call complete in
remoteOperationDone method, as I explained above, if master crashes after we
persist the state and then restarts, we need to call complete in the execute
method, so why not just follow the same pattern since you must have the code in
execute method?
And there is another advantage to prevent potential data race if you call
complete method in execute method, as the execute method will be called by
PEWorker, where we have a bunch of fencing ways to make sure that there is no
race.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ServerRemoteProcedure.java:
##########
@@ -80,6 +81,8 @@ public abstract class ServerRemoteProcedure extends
Procedure<MasterProcedureEnv
protected ServerName targetServer;
protected boolean dispatched;
protected boolean succ;
Review Comment:
Do it in your own convenience. We must have a state here, you can either add
a state but still keep this flag, or you can implement all the logic with the
state and drop this succ flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
