HBASE-24680 Refactor the checkAndMutate code on the server side #2094

brfrn169 · 2020-07-18T09:39:47Z

Added a new method checkAndMutate(CheckAndMutate) to Region/HRegion and deprecated the old methods checkAndPut/checkAndDelete/checkAndRowMutate
Added new coprocessor methods preCheckAndMutate/preCheckAndMutateAfterRowLock/postCheckAndMutate to RegionObserver and deprecated the old coprocessor methods preCheckAndPut/preCheckAndPutAfterRowLock/postCheckAndPut/preCheckAndDelete/preCheckAndDeleteAfterRowLock/postCheckAndDelete
Added a new metric for checkAndMutate

brfrn169 · 2020-07-22T01:49:40Z

@Apache9 Can you please review this when you get a chance?

joshelser

Generally looks OK to me. I don't have a lot of experience in the new CheckAndMutate objects, though.

joshelser · 2020-07-22T16:59:49Z

hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java

+
+    try {
+      if (mutations.size() == 1) {
+        if (mutations.get(0) instanceof Put) {


nit: Mutation m = mutations.get(0) and then use m for the rest of this block.

joshelser · 2020-07-22T17:13:35Z

hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java

+   */
+  default CheckAndMutateResult preCheckAndMutate(ObserverContext<RegionCoprocessorEnvironment> c,
+    CheckAndMutate checkAndMutate, CheckAndMutateResult result) throws IOException {
+    if (checkAndMutate.getAction() instanceof Put) {


Every time I see this, I want to suggest you use a visitor pattern to reduce the boilerplate, but that would require putting more logic on Put/Delete which not worth it. 🤷

Put/Delete is public so let's not do this...

joshelser · 2020-07-22T18:13:22Z

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java

  // checkAndMutate tests
  // ////////////////////////////////////////////////////////////////////////////
  @Test
+  @Deprecated


Probably don't need to mark deprecated tests in the future, but this is fine to leave, IMO.

brfrn169 · 2020-07-23T13:34:26Z

@joshelser Thank you very much for reviewing this!

I will wait for any other opinions/objections until tomorrow. If nothing, will commit this.

Apache9 · 2020-07-23T14:00:50Z

...oop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSource.java

+   * Update checkAndMutate histogram
+   * @param t time it took
+   */
+  void updateCheckAndMutate(long t);


After this change, do we still have checkAndPut and checkAndDelete?

Yes. This change keeps the existing metrics for checkAndPut and checkAndDelete for backward compatibility. This change doesn't break the existing tests for the metrics for checkAndPut and checkAndDelete.

Apache9 · 2020-07-23T14:01:43Z

hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java

+   */
+  default CheckAndMutateResult preCheckAndMutate(ObserverContext<RegionCoprocessorEnvironment> c,
+    CheckAndMutate checkAndMutate, CheckAndMutateResult result) throws IOException {
+    if (checkAndMutate.getAction() instanceof Put) {


Put/Delete is public so let's not do this...

Apache9 · 2020-07-23T14:02:55Z

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

+        throw new DoNotRetryIOException(
+          "Unsupported mutate type: " + mutation.getClass().getSimpleName().toUpperCase());
+      }
+    } catch (IllegalArgumentException e) {


Where do we throw this exception out? Catching a RuntimeException seems strange to me.

CheckAndMutate.Builder.build() calls preCheck() internally and it can throw IllegalArgumentException if the parameters are invalid:

hbase/hbase-client/src/main/java/org/apache/hadoop/hbase/client/CheckAndMutate.java

Lines 141 to 151 in 09e7ccd

private void preCheck(Row action) {

Preconditions.checkNotNull(action, "action (Put/Delete/RowMutations) is null");

if (!Bytes.equals(row, action.getRow())) {

throw new IllegalArgumentException("The row of the action (Put/Delete/RowMutations) <" +

Bytes.toStringBinary(action.getRow()) + "> doesn't match the original one <" +

Bytes.toStringBinary(this.row) + ">");

}

Preconditions.checkState(op != null || filter != null, "condition is null. You need to"

+ " specify the condition by calling ifNotExists/ifEquals/ifMatches before building a"

+ " CheckAndMutate object");

}

Then we'd better let the try catch for IllegalArgumentException only wrap the builder.build? And then we call checkAndMutate, without catching the IllegalArgumentException. I think this will be better to let others know why we may hit IllegalArgumentException.

Sure, thanks. Will do that as your review.

Apache9 · 2020-07-23T14:03:03Z

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

+        throw new DoNotRetryIOException("Unsupported mutate type: " +
+          mutation.getClass().getSimpleName().toUpperCase());
+      }
+    } catch (IllegalArgumentException e) {


hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

brfrn169

@Apache9 Thank you very much for taking a look at this!

I left some comments for your review. Can you please check them?

brfrn169 · 2020-07-24T09:43:46Z

...oop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSource.java

+   * Update checkAndMutate histogram
+   * @param t time it took
+   */
+  void updateCheckAndMutate(long t);


Yes. This change keeps the existing metrics for checkAndPut and checkAndDelete for backward compatibility. This change doesn't break the existing tests for the metrics for checkAndPut and checkAndDelete.

brfrn169 · 2020-07-24T09:49:53Z

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

+        throw new DoNotRetryIOException(
+          "Unsupported mutate type: " + mutation.getClass().getSimpleName().toUpperCase());
+      }
+    } catch (IllegalArgumentException e) {


CheckAndMutate.Builder.build() calls preCheck() internally and it can throw IllegalArgumentException if the parameters are invalid:

hbase/hbase-client/src/main/java/org/apache/hadoop/hbase/client/CheckAndMutate.java

Lines 141 to 151 in 09e7ccd

private void preCheck(Row action) {

Preconditions.checkNotNull(action, "action (Put/Delete/RowMutations) is null");

if (!Bytes.equals(row, action.getRow())) {

throw new IllegalArgumentException("The row of the action (Put/Delete/RowMutations) <" +

Bytes.toStringBinary(action.getRow()) + "> doesn't match the original one <" +

Bytes.toStringBinary(this.row) + ">");

}

Preconditions.checkState(op != null || filter != null, "condition is null. You need to"

+ " specify the condition by calling ifNotExists/ifEquals/ifMatches before building a"

+ " CheckAndMutate object");

}

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

brfrn169 · 2020-07-27T00:34:01Z

Not sure why QA was re-triggered.. The test failure in the last QA is not related to this patch because I ran the failed test locally and it was successful. Kicking QA again.

brfrn169 · 2020-07-27T23:12:05Z

Looks the QA is okay. Can you please take a look at this? @Apache9
Thanks.

brfrn169 · 2020-07-31T06:05:54Z

Ping @Apache9
Thanks.

Apache9 · 2020-07-31T07:15:14Z

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

+        throw new DoNotRetryIOException(
+          "Unsupported mutate type: " + mutation.getClass().getSimpleName().toUpperCase());
+      }
+    } catch (IllegalArgumentException e) {


Then we'd better let the try catch for IllegalArgumentException only wrap the builder.build? And then we call checkAndMutate, without catching the IllegalArgumentException. I think this will be better to let others know why we may hit IllegalArgumentException.

Apache9

+1.

Let's wait for the pre commit result.

Apache-HBase · 2020-08-01T06:31:14Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 31s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
		_ master Compile Tests _
+0 🆗	mvndep	0m 23s	Maven dependency ordering for branch
+1 💚	mvninstall	3m 40s	master passed
+1 💚	checkstyle	1m 59s	master passed
+1 💚	spotbugs	3m 33s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 13s	Maven dependency ordering for patch
+1 💚	mvninstall	3m 28s	the patch passed
+1 💚	checkstyle	0m 13s	The patch passed checkstyle in hbase-hadoop-compat
+1 💚	checkstyle	0m 32s	The patch passed checkstyle in hbase-client
+1 💚	checkstyle	1m 13s	hbase-server: The patch generated 0 new + 411 unchanged - 22 fixed = 411 total (was 433)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	hadoopcheck	11m 30s	Patch does not cause any errors with Hadoop 3.1.2 3.2.1.
+1 💚	spotbugs	4m 0s	the patch passed
		_ Other Tests _
+1 💚	asflicense	0m 37s	The patch does not generate ASF License warnings.
		39m 45s

Subsystem	Report/Notes
Docker	Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#2094
Optional Tests	dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle
uname	Linux e37a88cd64b6 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `7c4d66a`
Max. process+thread count	94 (vs. ulimit of 12500)
modules	C: hbase-hadoop-compat hbase-client hbase-server U: .
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2020-08-01T08:43:06Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 30s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+0 🆗	mvndep	0m 37s	Maven dependency ordering for branch
+1 💚	mvninstall	3m 26s	master passed
+1 💚	compile	1m 39s	master passed
+1 💚	shadedjars	5m 34s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	1m 19s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 16s	Maven dependency ordering for patch
+1 💚	mvninstall	3m 26s	the patch passed
+1 💚	compile	1m 38s	the patch passed
+1 💚	javac	1m 38s	the patch passed
+1 💚	shadedjars	5m 28s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	1m 17s	the patch passed
		_ Other Tests _
+1 💚	unit	0m 33s	hbase-hadoop-compat in the patch passed.
+1 💚	unit	1m 6s	hbase-client in the patch passed.
+1 💚	unit	140m 38s	hbase-server in the patch passed.
		170m 11s

Subsystem	Report/Notes
Docker	Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR	#2094
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 442c00b3f615 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `7c4d66a`
Default Java	1.8.0_232
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/testReport/
Max. process+thread count	4281 (vs. ulimit of 12500)
modules	C: hbase-hadoop-compat hbase-client hbase-server U: .
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f)
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2020-08-01T10:24:31Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	3m 54s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+0 🆗	mvndep	0m 26s	Maven dependency ordering for branch
+1 💚	mvninstall	5m 3s	master passed
+1 💚	compile	2m 12s	master passed
+1 💚	shadedjars	7m 12s	branch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	1m 1s	hbase-client in master failed.
-0 ⚠️	javadoc	0m 21s	hbase-hadoop-compat in master failed.
-0 ⚠️	javadoc	0m 47s	hbase-server in master failed.
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 16s	Maven dependency ordering for patch
+1 💚	mvninstall	5m 8s	the patch passed
+1 💚	compile	2m 8s	the patch passed
+1 💚	javac	2m 8s	the patch passed
+1 💚	shadedjars	6m 42s	patch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	0m 20s	hbase-hadoop-compat in the patch failed.
-0 ⚠️	javadoc	0m 30s	hbase-client in the patch failed.
-0 ⚠️	javadoc	0m 48s	hbase-server in the patch failed.
		_ Other Tests _
+1 💚	unit	0m 40s	hbase-hadoop-compat in the patch passed.
+1 💚	unit	1m 15s	hbase-client in the patch passed.
-1 ❌	unit	217m 12s	hbase-server in the patch failed.
		258m 45s

Subsystem	Report/Notes
Docker	Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR	#2094
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux e3442270b2fe 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `7c4d66a`
Default Java	2020-01-14
javadoc	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-client.txt
javadoc	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-hadoop-compat.txt
javadoc	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt
javadoc	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-hadoop-compat.txt
javadoc	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-client.txt
javadoc	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt
unit	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/testReport/
Max. process+thread count	3642 (vs. ulimit of 12500)
modules	C: hbase-hadoop-compat hbase-client hbase-server U: .
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2094/4/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f)
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

brfrn169 · 2020-08-01T11:56:46Z

I don' think the test failure in the last QA is related to this patch. I ran the failed tests locally and it was successful. Will merge this PR.

This comment has been minimized.

Sign in to view

brfrn169 force-pushed the HBASE-24680 branch from 983258e to 0982528 Compare July 18, 2020 18:56

brfrn169 self-assigned this Jul 18, 2020

brfrn169 requested review from Apache9, joshelser and saintstack July 18, 2020 19:41

This comment has been minimized.

Sign in to view

joshelser approved these changes Jul 22, 2020

View reviewed changes

brfrn169 force-pushed the HBASE-24680 branch from 0982528 to 3783a29 Compare July 23, 2020 13:17

Apache9 requested changes Jul 23, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

brfrn169 commented Jul 24, 2020

View reviewed changes

brfrn169 requested a review from Apache9 July 24, 2020 10:08

This comment has been minimized.

Sign in to view

Apache9 requested changes Jul 31, 2020

View reviewed changes

HBASE-24680 Refactor the checkAndMutate code on the server side

e3dfe5c

brfrn169 force-pushed the HBASE-24680 branch from 3783a29 to e3dfe5c Compare August 1, 2020 02:04

brfrn169 requested a review from Apache9 August 1, 2020 02:05

Apache9 approved these changes Aug 1, 2020

View reviewed changes

brfrn169 merged commit e22a2d2 into apache:master Aug 1, 2020

	private void preCheck(Row action) {
	Preconditions.checkNotNull(action, "action (Put/Delete/RowMutations) is null");
	if (!Bytes.equals(row, action.getRow())) {
	throw new IllegalArgumentException("The row of the action (Put/Delete/RowMutations) <" +
	Bytes.toStringBinary(action.getRow()) + "> doesn't match the original one <" +
	Bytes.toStringBinary(this.row) + ">");
	}
	Preconditions.checkState(op != null \|\| filter != null, "condition is null. You need to"
	+ " specify the condition by calling ifNotExists/ifEquals/ifMatches before building a"
	+ " CheckAndMutate object");
	}

HBASE-24680 Refactor the checkAndMutate code on the server side #2094

HBASE-24680 Refactor the checkAndMutate code on the server side #2094

Uh oh!

Conversation

brfrn169 commented Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brfrn169 commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshelser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brfrn169 commented Jul 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brfrn169 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

brfrn169 commented Jul 27, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

brfrn169 commented Jul 27, 2020

Uh oh!

brfrn169 commented Jul 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Apache9 left a comment

Choose a reason for hiding this comment

Uh oh!

Apache-HBase commented Aug 1, 2020

Uh oh!

Apache-HBase commented Aug 1, 2020

Uh oh!

Apache-HBase commented Aug 1, 2020

Uh oh!

brfrn169 commented Aug 1, 2020

Uh oh!

brfrn169 commented Jul 18, 2020 •

edited

Loading

brfrn169 commented Jul 22, 2020 •

edited

Loading