comnetwork edited a comment on pull request #4127:
URL: https://github.com/apache/hbase/pull/4127#issuecomment-1051963911
@Apache9 ， thank you very much for suggestions.
> I think we could just move the checks in onComplete method into the
synchronized block?
What do you mean? could you please explain more? you mean put the
`for (Map.Entry<Integer, MutableObject<Throwable>> entry :
replica2Error.entrySet()) {
Integer replicaId = entry.getKey();
Throwable error = entry.getValue().getValue();
if (error != null) {
if (maxSequenceId > lastFlushedSequenceIdToUse) {
LOG.warn(
"Failed to replicate to secondary replica {} for {}, since the
max sequence" +
" id of sunk entris is {}, which is greater than the last
flush SN {}," +
" we will stop replicating for a while and trigger a flush",
replicaId, primary, maxSequenceId, lastFlushedSequenceIdToUse,
error);
failed.add(replicaId);
} else {
LOG.warn(
"Failed to replicate to secondary replica {} for {}, since the
max sequence" +
" id of sunk entris is {}, which is less than or equal to the
last flush SN {}," +
" we will not stop replicating",
replicaId, primary, maxSequenceId, lastFlushedSequenceIdToUse,
error);
}
}
}` into the synchronized?
> And do we really need to expose so many internal things just for writing a
UT? Just expose the entries out, synchronized all the time to block the
onComplete method, and then trigger a flush? If synchronized is not easy to
check whether there are waiters, then you could change to use a Lock.
I expose these four internal things just for check more internal states are
as expected in the UT, in fact some checks is not really necessary for this
UT, just check for failedReplicas is enough, I could remove unnecessary check
and corresponding get internal state if you think exposing these internal state
is improper.
And In the UT, I should also make the unsynchronized block in `onComplete`
is not execute concurrently with the synchronized block in
`RegionReplicationSink.add`, the unsynchronized block should execute before
synchronized block in `RegionReplicationSink.add`, because there is an
unsynchronized block, just use the synchronized `entries` to control the
execute sequences seems not enough.
--
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]
