Uh oh!
There was an error while loading. Please reload this page.
HBASE-8458 Support for batch version of checkAndMutate() - #1648
Conversation
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.
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.
joshelser
left a comment
There was a problem hiding this comment.
So many new tests!!!
A couple of nit-picking questions on public API, but largely this looks great to me.
| CompletableFuture<Boolean> checkAndMutate(CheckAndMutate checkAndMutate); | ||
| /** | ||
| * Batch version of checkAndMutate. |
There was a problem hiding this comment.
Probably necessary to mention the consistency/expectations on this method.
By that, I mean, each CheckAndMutate is still individually atomic. They are batched only in the sense that they are sent to a RS in one RPC, but each CheckAndMutate operation is still executed atomically (and thus, each may fail independently of others). Is that correct?
There was a problem hiding this comment.
Yes, that's correct. I will add that to the JavaDoc of the new checkAndMutate methods for batch version. Thanks.
Uh oh!
There was an error while loading. Please reload this page.
| * @return the CheckAndMutate object | ||
| */ | ||
| public CheckAndMutate action(Row action) { | ||
| this.action = Preconditions.checkNotNull(action, "action is null"); |
There was a problem hiding this comment.
If this only supports Put, Delete, and RowMutations, should we check that up front?
There was a problem hiding this comment.
I will make Builder class and it will resolve this. Thanks.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| responseBuilder.addRegionActionResult(regionActionResultBuilder.build()); | ||
| quota.close(); |
There was a problem hiding this comment.
Maybe wrap this if condition branch with a finally to ensure the quote.close() happens?
There was a problem hiding this comment.
Yes, that's right. Thank you for pointing this out!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @Test(expected = IllegalArgumentException.class) | ||
| public void testCheckAndMutateWithoutAction() throws Throwable { | ||
| try (Table table = createTable()) { | ||
| table.checkAndMutate(new CheckAndMutate(ROWKEY) | ||
| .ifEquals(FAMILY, Bytes.toBytes("A"), Bytes.toBytes("a"))); | ||
| } | ||
| } | ||
| @Test(expected = IllegalArgumentException.class) | ||
| public void testCheckAndMutateWithoutCondition() throws Throwable { | ||
| try (Table table = createTable()) { | ||
| table.checkAndMutate(new CheckAndMutate(ROWKEY) | ||
| .action(new Put(ROWKEY).addColumn(FAMILY, Bytes.toBytes("D"), Bytes.toBytes("d")))); | ||
| } | ||
| } |
There was a problem hiding this comment.
In other places, we can get around this by making a Builder class in which the build() method takes the action as an argument.
e.g.
CheckAndMutate cm = new CheckAndMutate.Builder(ROWKEY)
.ifEquals(FAMILY, Bytes.toBytes("A"), Bytes.toBytes("a"))
.build(action);
This gets rid of some of the ambiguity of "you must call this method before you use this object". I don't feel super strongly about it, given how much code you'd have to change as a result :)
There was a problem hiding this comment.
I guess this could also solve the "unsupported action" type checking, as well.
You could overload the build(..) method to accept only the concrete types you allow: Put, Increment, etc.
There was a problem hiding this comment.
Thanks! Making Builder class is a good idea. I will make this change.
brfrn169
left a comment
There was a problem hiding this comment.
@joshelser Thank you very much for taking look at this! I will modify the PR for your review.
| CompletableFuture<Boolean> checkAndMutate(CheckAndMutate checkAndMutate); | ||
| /** | ||
| * Batch version of checkAndMutate. |
There was a problem hiding this comment.
Yes, that's correct. I will add that to the JavaDoc of the new checkAndMutate methods for batch version. Thanks.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| responseBuilder.addRegionActionResult(regionActionResultBuilder.build()); | ||
| quota.close(); |
There was a problem hiding this comment.
Yes, that's right. Thank you for pointing this out!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @Test(expected = IllegalArgumentException.class) | ||
| public void testCheckAndMutateWithoutAction() throws Throwable { | ||
| try (Table table = createTable()) { | ||
| table.checkAndMutate(new CheckAndMutate(ROWKEY) | ||
| .ifEquals(FAMILY, Bytes.toBytes("A"), Bytes.toBytes("a"))); | ||
| } | ||
| } | ||
| @Test(expected = IllegalArgumentException.class) | ||
| public void testCheckAndMutateWithoutCondition() throws Throwable { | ||
| try (Table table = createTable()) { | ||
| table.checkAndMutate(new CheckAndMutate(ROWKEY) | ||
| .action(new Put(ROWKEY).addColumn(FAMILY, Bytes.toBytes("D"), Bytes.toBytes("d")))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Thanks! Making Builder class is a good idea. I will make this change.
| * @return the CheckAndMutate object | ||
| */ | ||
| public CheckAndMutate action(Row action) { | ||
| this.action = Preconditions.checkNotNull(action, "action is null"); |
There was a problem hiding this comment.
I will make Builder class and it will resolve this. Thanks.
brfrn169
commented
May 23, 2020
@joshelser I have modified the patch for your review. Could you please review it? |
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
May 23, 2020
It looks like there were no test failures in the last QA but it reported the test failure. It seems like something errors other than test failures happened... |
brfrn169
commented
May 27, 2020
Ping @joshelser |
joshelser
commented
May 29, 2020
Sorry Toshi! Rough week :)
Looks like the Java process up and died. Can't tell if it was due to code you have changed though https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1648/3/PR_20JDK8_20Hadoop2_20Check_20Report/ Make sure to take a look at the Hadoop3 report and make sure you aren't introducing more javadoc warnings. I glanced at the one and it was the same number of warnings. A couple of more comments, but overall looks good. @Apache9 do you have interest in giving more feedback here? @busbey on your radar for 3.0.0? @saintstack FYI just as an all-around nice guy ;) |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
May 30, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 30, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 30, 2020
🎊 +1 overall
This message was automatically generated. |
brfrn169
commented
May 30, 2020
It looks like the unit tests are okay this time.
I checked the report and this patch is not introducing more javadoc warnings. |
joshelser
left a comment
There was a problem hiding this comment.
One final request, Toshi.
Let's have this also serve as one last call for other reviewers. If you have the cycles to put the extra interface annotations on CheckAndMutate$Builder, great. Otherwise, I'll do it on commit.
| /** | ||
| * A builder class for building a CheckAndMutate object. | ||
| */ | ||
| public static final class Builder { |
There was a problem hiding this comment.
Best practice to have interface audience/stability here too, I think.
brfrn169
commented
Jun 2, 2020
@joshelser Thank you for reviewing this! I just modified the patch for your review. Thanks. |
Apache-HBase
commented
Jun 2, 2020
🎊 +1 overall
This message was automatically generated. |
saintstack
left a comment
There was a problem hiding this comment.
I meant to review it and still haven't. Just did a quick skim. It looks great but its playing with fire (smile). Thanks @brfrn169
Uh oh!
There was an error while loading. Please reload this page.
| return checkAndMutate(CheckAndMutate.newBuilder(row) | ||
| .ifMatches(family, qualifier, op, value) | ||
| .timeRange(timeRange) | ||
| .build(put)); |
There was a problem hiding this comment.
I don't get how what is replaced maps to a checkAndMutate instance? There is no matching in previous impl?
There was a problem hiding this comment.
Ah yes, we don't need to replace maps to a checkAndMutate instance. I will revert this change.
| return checkAndMutate(CheckAndMutate.newBuilder(row) | ||
| .ifMatches(family, qualifier, op, value) | ||
| .timeRange(timeRange) | ||
| .build(delete)); |
Apache-HBase
commented
Jun 2, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 2, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2020
🎊 +1 overall
This message was automatically generated. |
brfrn169
commented
Jun 10, 2020
Ping @saintstack |
No description provided.