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 2020/02/04 08:00:40 UTC
[GitHub] [hbase] maoling opened a new pull request #1121: HBASE-20623:
[WIP]Introduce the helper method "getCellBuilder()" to Mutation
maoling opened a new pull request #1121: HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
URL: https://github.com/apache/hbase/pull/1121
- this is still a work in the progress, Plz don't review before removing the WIP tag.
- more details in the [HBASE-20623](https://issues.apache.org/jira/browse/HBASE-20623)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1121: HBASE-20623:
[WIP]Introduce the helper method "getCellBuilder()" to Mutation
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1121: HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
URL: https://github.com/apache/hbase/pull/1121#issuecomment-581926649
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 13s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 37s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 55s | master passed |
| +1 :green_heart: | compile | 3m 25s | master passed |
| +1 :green_heart: | checkstyle | 2m 33s | master passed |
| +0 :ok: | refguide | 5m 29s | branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | shadedjars | 5m 4s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 3m 51s | master passed |
| +0 :ok: | spotbugs | 4m 11s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 20m 51s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 34s | the patch passed |
| +1 :green_heart: | compile | 3m 21s | the patch passed |
| +1 :green_heart: | javac | 3m 21s | the patch passed |
| +1 :green_heart: | checkstyle | 2m 31s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +0 :ok: | refguide | 5m 19s | patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | shadedjars | 5m 5s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 29s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 3m 50s | the patch passed |
| +1 :green_heart: | findbugs | 21m 11s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 237m 7s | root in the patch passed. |
| +1 :green_heart: | asflicense | 1m 48s | The patch does not generate ASF License warnings. |
| | | 360m 3s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1121/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1121 |
| JIRA Issue | HBASE-20623 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile refguide |
| uname | Linux 75a924866bd5 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1121/out/precommit/personality/provided.sh |
| git revision | master / bb14bdad62 |
| Default Java | 1.8.0_181 |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1121/1/artifact/out/branch-site/book.html |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1121/1/artifact/out/patch-site/book.html |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1121/1/testReport/ |
| Max. process+thread count | 5381 (vs. ulimit of 10000) |
| modules | C: hbase-client hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1121/1/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1121:
HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1121: HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
URL: https://github.com/apache/hbase/pull/1121#discussion_r374947794
##########
File path: src/main/asciidoc/_chapters/datamodel.adoc
##########
@@ -471,6 +471,26 @@ Caution: the version timestamp is used internally by HBase for things like time-
It's usually best to avoid setting this timestamp yourself.
Prefer using a separate timestamp attribute of the row, or have the timestamp as a part of the row key, or both.
+===== Cell Version Example
+
+The following Put uses a method getCellBuilder() to get a CellBuilder instance
+that already has relevant Type and Row set.
+
+[source,java]
+----
+
+public static final byte[] CF = "cf".getBytes();
+public static final byte[] ATTR = "attr".getBytes();
+...
+
+Put put = new Put(Bytes.toBytes(row));
+put.add(put.getCellBuilder().setQualifier(ATTR)
+ .setFamily(CF)
+ .setValue(Bytes.toBytes(data))
+ .build());
Review comment:
Could write this as put.addColumn()? It'd be easier? What you see advantage of this being able to do it by Cell? Will it confuse the user?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1121:
HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1121: HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
URL: https://github.com/apache/hbase/pull/1121#discussion_r374946559
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMutationGetCellBuilder.java
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.client;
+
+import static org.junit.Assert.assertTrue;
+
+import java.util.Arrays;
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.CellBuilder;
+import org.apache.hadoop.hbase.CellUtil;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.testclassification.ClientTests;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestName;
+
+@Category({MediumTests.class, ClientTests.class})
+public class TestMutationGetCellBuilder {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestMutationGetCellBuilder.class);
+
+ private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
+
+ @Rule
+ public TestName name = new TestName();
+
+ @BeforeClass
+ public static void setUpBeforeClass() throws Exception {
+ TEST_UTIL.startMiniCluster();
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() throws Exception {
+ TEST_UTIL.shutdownMiniCluster();
+ }
+
+ @Test
+ public void testMutationGetCellBuilder() throws Exception {
+ final TableName tableName = TableName.valueOf(name.getMethodName());
+ final byte[] rowKey = Bytes.toBytes("12345678");
+ final byte[] uselessRowKey = Bytes.toBytes("123");
+ final byte[] family = Bytes.toBytes("cf");
+ final byte[] qualifier = Bytes.toBytes("foo");
+ final long now = System.currentTimeMillis();
+ try (Table table = TEST_UTIL.createTable(tableName, family)) {
+ TEST_UTIL.waitTableAvailable(tableName.getName(), 5000);
+ // put one row
+ Put put = new Put(rowKey);
+ CellBuilder cellBuilder = put.getCellBuilder().setQualifier(qualifier)
+ .setFamily(family).setValue(Bytes.toBytes("bar")).setTimestamp(now);
+ //setRow is useless
+ cellBuilder.setRow(uselessRowKey);
Review comment:
Should it throw an exception? Maybe it can't?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] maoling commented on a change in pull request #1121:
HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1121: HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
URL: https://github.com/apache/hbase/pull/1121#discussion_r375085410
##########
File path: src/main/asciidoc/_chapters/datamodel.adoc
##########
@@ -471,6 +471,26 @@ Caution: the version timestamp is used internally by HBase for things like time-
It's usually best to avoid setting this timestamp yourself.
Prefer using a separate timestamp attribute of the row, or have the timestamp as a part of the row key, or both.
+===== Cell Version Example
+
+The following Put uses a method getCellBuilder() to get a CellBuilder instance
+that already has relevant Type and Row set.
+
+[source,java]
+----
+
+public static final byte[] CF = "cf".getBytes();
+public static final byte[] ATTR = "attr".getBytes();
+...
+
+Put put = new Put(Bytes.toBytes(row));
+put.add(put.getCellBuilder().setQualifier(ATTR)
+ .setFamily(CF)
+ .setValue(Bytes.toBytes(data))
+ .build());
Review comment:
@saintstack Thanks for your review.
- Yes, `put.addColumn()` can have the same effect.
- this design was discussed in this [email thread](https://lists.apache.org/thread.html/d05bfaa0134502a47f6e1aca56cb0b096d4dd32ddefbbdf28db4952a@%3Cdev.hbase.apache.org%3E) which had a user case provided by Sean Busbey. AFAIU, it wants to simplify the `put.add(cell)` api, because sometimes when users use this cell api:
```
CellBuilder cb = CellBuilderFactory.create(CellBuilderType.SHALLOW_COPY);
cb.setRow(Bytes.toBytes("row3"));
cb.setFamily(Bytes.toBytes("cf"));
cb.setQualifier("qualifier1".getBytes());
cb.setValue(Bytes.toBytes("mjj2"));
cb.setType(Type.Put);
Cell cell = cb.build();
Put p = new Put(Bytes.toBytes("row3"));
p.add(cell);
```
`cb.setType(Type.Put)` is a little redundant and `getCellBuilder()` can help users to reuse the row, even Family and Qualifier they set last time to make the code short and clean.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1121:
HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1121: HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
URL: https://github.com/apache/hbase/pull/1121#discussion_r377224907
##########
File path: src/main/asciidoc/_chapters/datamodel.adoc
##########
@@ -471,6 +471,26 @@ Caution: the version timestamp is used internally by HBase for things like time-
It's usually best to avoid setting this timestamp yourself.
Prefer using a separate timestamp attribute of the row, or have the timestamp as a part of the row key, or both.
+===== Cell Version Example
+
+The following Put uses a method getCellBuilder() to get a CellBuilder instance
+that already has relevant Type and Row set.
+
+[source,java]
+----
+
+public static final byte[] CF = "cf".getBytes();
+public static final byte[] ATTR = "attr".getBytes();
+...
+
+Put put = new Put(Bytes.toBytes(row));
+put.add(put.getCellBuilder().setQualifier(ATTR)
+ .setFamily(CF)
+ .setValue(Bytes.toBytes(data))
+ .build());
Review comment:
I see. Usability. A fix that came of discussion. Good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack merged pull request #1121: HBASE-20623:
[WIP]Introduce the helper method "getCellBuilder()" to Mutation
Posted by GitBox <gi...@apache.org>.
saintstack merged pull request #1121: HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
URL: https://github.com/apache/hbase/pull/1121
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1121:
HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1121: HBASE-20623: [WIP]Introduce the helper method "getCellBuilder()" to Mutation
URL: https://github.com/apache/hbase/pull/1121#discussion_r374943510
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Mutation.java
##########
@@ -784,6 +787,104 @@ Mutation add(Cell cell) throws IOException {
return this;
}
+ /**
+ * get a CellBuilder instance that already has relevant Type and Row set.
+ * @param cellBuilderType e.g CellBuilderType.SHALLOW_COPY
+ * @return CellBuilder which already has relevant Type and Row set.
+ */
+ public abstract CellBuilder getCellBuilder(CellBuilderType cellBuilderType);
+
+ /**
+ * get a CellBuilder instance that already has relevant Type and Row set.
+ * the default CellBuilderType is CellBuilderType.SHALLOW_COPY
+ * @return CellBuilder which already has relevant Type and Row set.
+ */
+ public CellBuilder getCellBuilder() {
+ return getCellBuilder(CellBuilderType.SHALLOW_COPY);
+ }
+
+ /**
+ * get a CellBuilder instance that already has relevant Type and Row set.
+ * @param cellBuilderType e.g CellBuilderType.SHALLOW_COPY
+ * @param cellType e.g Cell.Type.Put
+ * @return CellBuilder which already has relevant Type and Row set.
+ */
+ protected CellBuilder getCellBuilder(CellBuilderType cellBuilderType, Cell.Type cellType) {
Review comment:
So you shutdown setting row, type, etc., with the below?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services