You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2022/05/11 14:02:05 UTC
[GitHub] [hbase] Apache9 commented on a diff in pull request #4392: HBASE-26993 Make the new framework for region replication could work …
Apache9 commented on code in PR #4392:
URL: https://github.com/apache/hbase/pull/4392#discussion_r870295934
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -3372,8 +3372,8 @@ public abstract void prepareMiniBatchOperations(
* Write mini-batch operations to MemStore
*/
public abstract WriteEntry writeMiniBatchOperationsToMemStore(
- final MiniBatchOperationInProgress<Mutation> miniBatchOp, final WriteEntry writeEntry)
- throws IOException;
+ final MiniBatchOperationInProgress<Mutation> miniBatchOp, final WriteEntry writeEntry,
+ long now) throws IOException;
Review Comment:
Why this change?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -3606,24 +3610,25 @@ protected MiniBatchOperationInProgress<Mutation> createMiniBatch(final int lastI
@Override
public boolean visit(int index) throws IOException {
Mutation m = getMutation(index);
+ // the batch may contain multiple nonce keys (replay case). If so, write WALEdit for each.
+ // Given how nonce keys are originally written, these should be contiguous.
+ // They don't have to be, it will still work, just write more WALEdits than needed.
+ long nonceGroup = getNonceGroup(index);
+ long nonce = getNonce(index);
// we use durability of the original mutation for the mutation passed by CP.
if (region.getEffectiveDurability(m.getDurability()) == Durability.SKIP_WAL) {
region.recordMutationWithoutWal(m.getFamilyCellMap());
+ miniBatchOp.addSkipWALMutation(new NonceKey(nonceGroup, nonce), familyCellMaps[index]);
Review Comment:
Ah, OK, the trick is here. Do we need to check whether region replication is on?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -7947,6 +8000,58 @@ private WriteEntry doWALAppend(WALEdit walEdit, Durability durability, List<UUID
}
throw ioe;
}
+ }
+
+ /**
+ * Attach {@link RegionReplicationSink#add} to the mvcc writeEntry for replicating to region
+ * replica.
+ */
+ private void attachReplicateRegionReplicaInWALAppend(BatchOperation<?> batchOp,
+ MiniBatchOperationInProgress<Mutation> miniBatchOp,
+ NonceKey nonceKey, WALKeyImpl walKey, WALEdit walEdit, WriteEntry writeEntry)
+ throws IOException {
+ if (!regionReplicationSink.isPresent()) {
+ return;
+ }
+ final WALEdit walEditToUse =
+ getWALEditForReplicateRegionReplica(batchOp, miniBatchOp, nonceKey, walEdit);
+ final ServerCall<?> rpcCall = RpcServer.getCurrentServerCallWithCellScanner().orElse(null);
+ regionReplicationSink.ifPresent(sink -> writeEntry.attachCompletionAction(() -> {
+ sink.add(walKey, walEditToUse, rpcCall);
+ }));
+ }
+
+ /**
+ * Here is for HBASE-26993 case 1,partial {@link Mutation}s are {@link Durability#SKIP_WAL}.In
+ * order to make the new framework for region replication could work for SKIP_WAL, we must add the
+ * {@link Mutation} which {@link Mutation#getDurability} is {@link Durability#SKIP_WAL} to the
+ * {@link WALEdit} used for replicating to region replica.
+ */
+ private WALEdit getWALEditForReplicateRegionReplica(BatchOperation<?> batchOp,
+ MiniBatchOperationInProgress<Mutation> miniBatchOp,
+ NonceKey nonceKey, WALEdit walEdit) throws IOException {
+ /**
+ * Here there is no need to consider there are SKIP_WAL {@link Mutation}s which are not covered
Review Comment:
If nonceKey is always the same, why we need to pass in the nonceKey when getting the columnFamilyToCellsList?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -7947,6 +8000,58 @@ private WriteEntry doWALAppend(WALEdit walEdit, Durability durability, List<UUID
}
throw ioe;
}
+ }
+
+ /**
+ * Attach {@link RegionReplicationSink#add} to the mvcc writeEntry for replicating to region
+ * replica.
+ */
+ private void attachReplicateRegionReplicaInWALAppend(BatchOperation<?> batchOp,
+ MiniBatchOperationInProgress<Mutation> miniBatchOp,
+ NonceKey nonceKey, WALKeyImpl walKey, WALEdit walEdit, WriteEntry writeEntry)
+ throws IOException {
+ if (!regionReplicationSink.isPresent()) {
+ return;
+ }
+ final WALEdit walEditToUse =
+ getWALEditForReplicateRegionReplica(batchOp, miniBatchOp, nonceKey, walEdit);
+ final ServerCall<?> rpcCall = RpcServer.getCurrentServerCallWithCellScanner().orElse(null);
+ regionReplicationSink.ifPresent(sink -> writeEntry.attachCompletionAction(() -> {
+ sink.add(walKey, walEditToUse, rpcCall);
+ }));
+ }
+
+ /**
+ * Here is for HBASE-26993 case 1,partial {@link Mutation}s are {@link Durability#SKIP_WAL}.In
+ * order to make the new framework for region replication could work for SKIP_WAL, we must add the
+ * {@link Mutation} which {@link Mutation#getDurability} is {@link Durability#SKIP_WAL} to the
+ * {@link WALEdit} used for replicating to region replica.
+ */
+ private WALEdit getWALEditForReplicateRegionReplica(BatchOperation<?> batchOp,
+ MiniBatchOperationInProgress<Mutation> miniBatchOp,
+ NonceKey nonceKey, WALEdit walEdit) throws IOException {
+ /**
+ * Here there is no need to consider there are SKIP_WAL {@link Mutation}s which are not covered
+ * by {@link HRegion#doWALAppend} because for primary region,only {@link MutationBatchOperation}
+ * is used and {@link NonceKey} is all the same for {@link Mutation}s in
+ * {@link MutationBatchOperation}.
+ */
+ List<Map<byte[], List<Cell>>> columnFamilyToCellsList = miniBatchOp.getSkipMutations(nonceKey);
+ if(columnFamilyToCellsList == null || columnFamilyToCellsList.isEmpty()) {
+ return walEdit;
+ }
+
+ /**
+ * Create a new WALEdit for replicating to region replica,and add the {@link Mutation} which
+ * {@link Mutation#getDurability} is {@link Durability#SKIP_WAL}
+ */
+ WALEdit newWALEdit = new WALEdit(walEdit);
+ for (Map<byte[], List<Cell>> columnFamilyToCells : columnFamilyToCellsList) {
Review Comment:
So does the order matter here? I mean we already have some rows and cells in the WALEdit, and then we add the missed ones to it, then how can we preserve the order?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -4142,15 +4147,58 @@ private static long getLongValue(final Cell cell) throws DoNotRetryIOException {
@Override
public WriteEntry writeMiniBatchOperationsToMemStore(
- final MiniBatchOperationInProgress<Mutation> miniBatchOp, @Nullable WriteEntry writeEntry)
- throws IOException {
+ final MiniBatchOperationInProgress<Mutation> miniBatchOp, @Nullable WriteEntry writeEntry,
+ long now) throws IOException {
+ boolean newWriteEntry = false;
if (writeEntry == null) {
writeEntry = region.mvcc.begin();
+ newWriteEntry = true;
}
super.writeMiniBatchOperationsToMemStore(miniBatchOp, writeEntry.getWriteNumber());
+ if (newWriteEntry) {
Review Comment:
Mind explain a bit why we only need to do this when newWriteEntry is true?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MiniBatchOperationInProgress.java:
##########
@@ -182,4 +195,25 @@ public int getNumOfAppends() {
public void incrementNumOfAppends() {
this.numOfAppends += 1;
}
+
+ public void addSkipWALMutation(NonceKey nonceKey, Map<byte[], List<Cell>> columnFamilyToCells) {
+ if (columnFamilyToCells == null) {
+ return;
+ }
+ if (this.nonceKeyToSkipWALMutations == null) {
+ this.nonceKeyToSkipWALMutations = new HashMap<>();
+ }
+ List<Map<byte[], List<Cell>>> skipWALMuations =
+ this.nonceKeyToSkipWALMutations.computeIfAbsent(nonceKey, (key) -> new ArrayList<>());
Review Comment:
Unnecessary '()' around the 'key'
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org