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/01/17 21:51:22 UTC
[GitHub] [hbase] saintstack opened a new pull request #1062: HBASE-23705 Add
CellComparator to HFileContext
saintstack opened a new pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062
Codecs don't have access to what CellComparator to use. Backfill.
M hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java
Adds a new compareRows with default implementation that takes a ByteBuffer.
Needed by the index in a block encoder implementation.
M hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
Adds implementation for meta of new compareRows method. Adds utility
method for figuring comparator based off tablename.
M hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/AbstractDataBlockEncoder.java
M hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
M hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexCodecV1.java
M hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexSeekerV1.java
Comparator is in context. Remove redundant handling.
M hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
Comparator is in context. Remove redundant handling. Clean javadoc.
M hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
Clean javadoc.
M hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexEncoderV1.java
Cache context so can use it to get comparator to use later.
M hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContext.java
Cache cellcomparator to use. Javdoc on diff between HFileContext and
HFileInfo.
M hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContextBuilder.java
Add CellComparator
M hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat2.java
M hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
M hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java
M hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterImpl.java
M hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java
Remove comparator caching. Get from context instead.
M hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/FixedFileTrailer.java
Skip a reflection if we can.
M hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileInfo.java
Javadoc. Removed unused filed.
----------------------------------------------------------------
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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368310694
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
##########
@@ -361,7 +361,7 @@ static HFileBlock createFromBuff(ByteBuff buf, boolean usesHBaseChecksum, final
// This constructor is called when we deserialize a block from cache and when we read a block in
// from the fs. fileCache is null when deserialized from cache so need to make up one.
HFileContextBuilder fileContextBuilder =
- fileContext != null ? new HFileContextBuilder(fileContext) : new HFileContextBuilder();
+ fileContext != null ? new HFileContextBuilder(fileContext): new HFileContextBuilder();
Review comment:
NIT: Usually there should be a whitespace before the `:`. Thought it is already part of the Checkstyle ruleset.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r369150752
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContext.java
##########
@@ -114,6 +118,11 @@ public HFileContext(HFileContext context) {
this.hfileName = hfileName;
this.columnFamily = columnFamily;
this.tableName = tableName;
+ // If no cellComparator specified, make a guess based off tablename. If hbase:meta, then should
+ // be the meta table comparator. Comparators are per table.
+ this.cellComparator = cellComparator != null? cellComparator:
Review comment:
Implemented.
----------------------------------------------------------------
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] anoopsjohn commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368653450
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexSeekerV1.java
##########
@@ -154,19 +153,6 @@ private int binarySearch(Cell seekCell, boolean seekBefore) {
}
}
- private int compareRows(ByteBuffer row, Cell seekCell) {
Review comment:
I understood ur arg now. Make sense... The MetaCellComparator was doing on heap copy. So ur impl there is ok as of now. May be we can see whether we can avoid some of those. But can be another jira. +1 for doing this way boss.
----------------------------------------------------------------
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 #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-577139888
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 30s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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 20 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 36s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 10s | master passed |
| +1 :green_heart: | compile | 1m 46s | master passed |
| +1 :green_heart: | checkstyle | 1m 57s | master passed |
| +1 :green_heart: | shadedjars | 4m 36s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 20s | master passed |
| +0 :ok: | spotbugs | 4m 32s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 5s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 59s | the patch passed |
| +1 :green_heart: | compile | 1m 46s | the patch passed |
| +1 :green_heart: | javac | 1m 46s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 23s | hbase-common: The patch generated 0 new + 17 unchanged - 32 fixed = 17 total (was 49) |
| +1 :green_heart: | checkstyle | 1m 8s | hbase-server: The patch generated 0 new + 55 unchanged - 155 fixed = 55 total (was 210) |
| -1 :x: | checkstyle | 0m 19s | hbase-mapreduce: The patch generated 1 new + 0 unchanged - 18 fixed = 1 total (was 18) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 4m 34s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 15m 36s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 17s | the patch passed |
| +1 :green_heart: | findbugs | 6m 23s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 21s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 156m 42s | hbase-server in the patch passed. |
| -1 :x: | unit | 24m 56s | hbase-mapreduce in the patch failed. |
| +1 :green_heart: | asflicense | 2m 1s | The patch does not generate ASF License warnings. |
| | | 252m 59s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.replication.TestVerifyReplication |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1062 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 0d12b8919210 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1062/out/precommit/personality/provided.sh |
| git revision | master / 2ed81c645b |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/4/artifact/out/diff-checkstyle-hbase-mapreduce.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/4/artifact/out/patch-unit-hbase-mapreduce.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/4/testReport/ |
| Max. process+thread count | 5598 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-server hbase-mapreduce U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/4/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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-577493606
Unit test is known issue (HBASE-23722). The checkstyle is the method-too-large. Merging.
----------------------------------------------------------------
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] anoopsjohn commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368413179
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/FixedFileTrailer.java
##########
@@ -624,19 +624,22 @@ private String getHBase1CompatibleName(final String comparator) {
return comparatorKlass;
}
- public static CellComparator createComparator(
- String comparatorClassName) throws IOException {
+ static CellComparator createComparator(String comparatorClassName) throws IOException {
+ if (comparatorClassName.equals(CellComparatorImpl.COMPARATOR.getClass().getName())) {
+ return CellComparatorImpl.COMPARATOR;
+ } else if (comparatorClassName.equals(
+ CellComparatorImpl.META_COMPARATOR.getClass().getName())) {
+ return CellComparatorImpl.META_COMPARATOR;
+ }
Review comment:
With out this also (with below way of get class based on the name) it will work? U just added a short circuit here? Or u fixing some issue here with below getComparatorClass(comparatorClassName)?
----------------------------------------------------------------
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 #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-575884015
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 34s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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 23 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 40s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 14s | master passed |
| +1 :green_heart: | compile | 1m 49s | master passed |
| +1 :green_heart: | checkstyle | 2m 4s | master passed |
| +1 :green_heart: | shadedjars | 4m 41s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 17s | master passed |
| +0 :ok: | spotbugs | 4m 36s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 11s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 59s | the patch passed |
| +1 :green_heart: | compile | 1m 47s | the patch passed |
| +1 :green_heart: | javac | 1m 47s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 25s | hbase-common: The patch generated 0 new + 14 unchanged - 18 fixed = 14 total (was 32) |
| -1 :x: | checkstyle | 1m 19s | hbase-server: The patch generated 2 new + 465 unchanged - 5 fixed = 467 total (was 470) |
| +1 :green_heart: | checkstyle | 0m 20s | hbase-mapreduce: The patch generated 0 new + 17 unchanged - 1 fixed = 17 total (was 18) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 4m 44s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 16m 2s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 17s | the patch passed |
| +1 :green_heart: | findbugs | 6m 24s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 18s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 155m 24s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 15m 44s | hbase-mapreduce in the patch passed. |
| +1 :green_heart: | asflicense | 1m 50s | The patch does not generate ASF License warnings. |
| | | 243m 29s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1062 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 4efb90622411 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1062/out/precommit/personality/provided.sh |
| git revision | master / 70c8a5d939 |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/artifact/out/diff-checkstyle-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/testReport/ |
| Max. process+thread count | 5574 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-server hbase-mapreduce U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368310270
##########
File path: hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat2.java
##########
@@ -424,15 +423,14 @@ private WriterLength getNewWriter(byte[] tableName, byte[] family, Configuration
HFileContext hFileContext = contextBuilder.build();
if (null == favoredNodes) {
wl.writer =
- new StoreFileWriter.Builder(conf, CacheConfig.DISABLED, fs)
- .withOutputDir(familydir).withBloomType(bloomType)
- .withComparator(CellComparator.getInstance()).withFileContext(hFileContext).build();
+ new StoreFileWriter.Builder(conf, CacheConfig.DISABLED, fs).
+ withOutputDir(familydir).withBloomType(bloomType).
Review comment:
NIT: Most of the time we use a leading `.`.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368323728
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -387,4 +409,23 @@ public Comparator getSimpleComparator() {
public Comparator getSimpleComparator() {
return new BBKVComparator(this);
}
+
+ /**
+ * Utility method that makes a guess at comparator to use based off passed tableName.
+ * Use in extreme when no comparator specified.
+ * @return CellComparator to use going off the <code>tableName</code> passed.
+ */
+ public static CellComparator getCellComparator(TableName tableName) {
+ return getCellComparator(tableName.toBytes());
+ }
+
+ /**
+ * Utility method that makes a guess at comparator to use based off passed tableName.
+ * Use in extreme when no comparator specified.
+ * @return CellComparator to use going off the <code>tableName</code> passed.
Review comment:
Will do.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368657173
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexSeekerV1.java
##########
@@ -154,19 +153,6 @@ private int binarySearch(Cell seekCell, boolean seekBefore) {
}
}
- private int compareRows(ByteBuffer row, Cell seekCell) {
Review comment:
Thanks @anoopsjohn
Will put up new patch that implements your suggestions and those of @HorizonNet
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r369160858
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
##########
@@ -85,36 +89,37 @@
@Test
public void testUsingMetaAndBinary() throws IOException {
- FileSystem filesystem = FileSystem.get(conf);
Path rootdir = UTIL.getDataTestDirOnTestFS();
// Up flush size else we bind up when we use default catalog flush of 16k.
- TableDescriptorBuilder metaBuilder = UTIL.getMetaTableDescriptorBuilder()
- .setMemStoreFlushSize(64 * 1024 * 1024);
-
+ TableDescriptors tds = new FSTableDescriptors(UTIL.getConfiguration());
+ TableDescriptorBuilder metaBuilder = TableDescriptorBuilder.
+ newBuilder(tds.get(TableName.META_TABLE_NAME)).setMemStoreFlushSize(64 * 1024 * 1024);
+ TableDescriptor td = metaBuilder.build();
HRegion mr = HBaseTestingUtility.createRegionAndWAL(HRegionInfo.FIRST_META_REGIONINFO,
- rootdir, this.conf, metaBuilder.build());
+ rootdir, this.conf, td);
try {
// Write rows for three tables 'A', 'B', and 'C'.
for (char c = 'A'; c < 'D'; c++) {
HTableDescriptor htd = new HTableDescriptor(TableName.valueOf("" + c));
final int last = 128;
final int interval = 2;
for (int i = 0; i <= last; i += interval) {
- HRegionInfo hri = new HRegionInfo(htd.getTableName(),
- i == 0 ? HConstants.EMPTY_BYTE_ARRAY : Bytes.toBytes((byte) i),
- i == last ? HConstants.EMPTY_BYTE_ARRAY : Bytes.toBytes((byte) i + interval));
-
+ RegionInfo hri = RegionInfoBuilder.newBuilder(htd.getTableName()).
+ setStartKey(i == 0? HConstants.EMPTY_BYTE_ARRAY: Bytes.toBytes((byte)i)).
Review comment:
Implemented.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368618259
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -377,6 +378,27 @@ private static int compareRows(byte[] left, int loffset, int llength, byte[] rig
return result;
}
+ @Override
+ public int compareRows(ByteBuffer row, Cell cell) {
+ byte [] array;
+ int offset;
+ int len = row.remaining();
+ if (row.hasArray()) {
+ array = row.array();
+ offset = row.position() + row.arrayOffset();
+ } else {
+ // This is awful, we copy the row array if offheap just so we can do a compare.
+ // We do this elsewhere too when Cell is backed by an offheap ByteBuffer.
+ // Needs fixing. TODO.
Review comment:
> Only when a user tries to do getRowArray on a BB backed cell we try to copy the row part and then do the comparsion. In that case he should also be using getRowOfset() only which would be
I should have been more specific. Lets make it so no need to copy-to-compare ever.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368615564
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileInfo.java
##########
@@ -380,7 +386,8 @@ private HFileContext createHFileContext(Path path,
HFileContextBuilder builder = new HFileContextBuilder()
.withHBaseCheckSum(true)
.withHFileName(path.getName())
- .withCompression(trailer.getCompressionCodec());
+ .withCompression(trailer.getCompressionCodec())
+ .withCellComparator(trailer.createComparator(trailer.getComparatorClassName()));
Review comment:
I want to keep context immutable so it is safe to use in any context. It is why I removed the setter methods. I think it fine building a new one here rather than reuse. Its once per file reader open. Seems fine.
Let me add your getCellComparator suggestion.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368615118
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexSeekerV1.java
##########
@@ -131,8 +131,7 @@ private int binarySearch(Cell seekCell, boolean seekBefore) {
int comp = 0;
while (low <= high) {
mid = low + ((high - low) >> 1);
- ByteBuffer row = getRow(mid);
- comp = compareRows(row, seekCell);
+ comp = this.cellComparator.compareRows(getRow(mid), seekCell);
Review comment:
There is a similar array-based compare rows in CellComparator.
See above for my argument that encoders should not each have to do their own figuring of cell compare.
Notice how much cleaner the code in here is now?
----------------------------------------------------------------
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] ramkrish86 commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
ramkrish86 commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368445204
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -377,6 +378,27 @@ private static int compareRows(byte[] left, int loffset, int llength, byte[] rig
return result;
}
+ @Override
+ public int compareRows(ByteBuffer row, Cell cell) {
+ byte [] array;
+ int offset;
+ int len = row.remaining();
+ if (row.hasArray()) {
+ array = row.array();
+ offset = row.position() + row.arrayOffset();
+ } else {
+ // This is awful, we copy the row array if offheap just so we can do a compare.
+ // We do this elsewhere too when Cell is backed by an offheap ByteBuffer.
+ // Needs fixing. TODO.
Review comment:
Are you sure on this STack. All the impl of the BB vs Array (vice versa) or BB vs BB all happens inside the BBUtils method. There we are clearly ensuring that we either call getXXxOffset() or getXXXPosition() so that we avoide copying onheap. Only when a user tries to do getRowArray on a BB backed cell we try to copy the row part and then do the comparsion. In that case he should also be using getRowOfset() only which would be 0/.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368604999
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java
##########
@@ -80,6 +83,24 @@ static CellComparator getInstance() {
*/
int compareRows(Cell cell, byte[] bytes, int offset, int length);
+ /**
+ * @param row ByteBuffer that wraps a row; will read from current position and will reading all
+ * remaining; will not disturb the ByteBuffer internal state.
+ * @return greater than 0 if leftCell is bigger, less than 0 if rightCell is bigger, 0 if both
+ * cells are equal
+ */
+ default int compareRows(ByteBuffer row, Cell cell) {
Review comment:
Yes.
Javadoc tries to make this explicit. Should I add more?
" * @param row ByteBuffer that wraps a row; will read from current position and will reading all
* remaining; will not disturb the ByteBuffer internal state."
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r369159791
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java
##########
@@ -276,8 +275,8 @@ private Path writeStoreFile() throws IOException {
Path storeFileParentDir = new Path(TEST_UTIL.getDataTestDir(), "TestHFile");
HFileContext meta = new HFileContextBuilder().withBlockSize(64 * 1024).build();
StoreFileWriter sfw =
- new StoreFileWriter.Builder(conf, fs).withOutputDir(storeFileParentDir)
- .withComparator(CellComparatorImpl.COMPARATOR).withFileContext(meta).build();
+ new StoreFileWriter.Builder(conf, fs).withOutputDir(storeFileParentDir).
+ withFileContext(meta).build();
Review comment:
Done
----------------------------------------------------------------
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 #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-577396513
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 18s | 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 20 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 34s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 46s | master passed |
| +1 :green_heart: | compile | 1m 51s | master passed |
| +1 :green_heart: | checkstyle | 59m 15s | master passed |
| +1 :green_heart: | shadedjars | 5m 2s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 16s | master passed |
| +0 :ok: | spotbugs | 5m 24s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 7m 2s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 6m 35s | the patch passed |
| +1 :green_heart: | compile | 2m 19s | the patch passed |
| +1 :green_heart: | javac | 2m 19s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 27s | hbase-common: The patch generated 0 new + 17 unchanged - 32 fixed = 17 total (was 49) |
| +1 :green_heart: | checkstyle | 1m 25s | hbase-server: The patch generated 0 new + 55 unchanged - 155 fixed = 55 total (was 210) |
| -1 :x: | checkstyle | 0m 20s | hbase-mapreduce: The patch generated 1 new + 0 unchanged - 18 fixed = 1 total (was 18) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 36s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 18m 44s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 15s | the patch passed |
| +1 :green_heart: | findbugs | 7m 2s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 12s | hbase-common in the patch passed. |
| -1 :x: | unit | 169m 4s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 18m 44s | hbase-mapreduce in the patch passed. |
| +1 :green_heart: | asflicense | 1m 30s | The patch does not generate ASF License warnings. |
| | | 326m 28s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.security.provider.TestCustomSaslAuthenticationProvider |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/5/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1062 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 486397b1d226 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-1062/out/precommit/personality/provided.sh |
| git revision | master / 11b7ecb3af |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/5/artifact/out/diff-checkstyle-hbase-mapreduce.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/5/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/5/testReport/ |
| Max. process+thread count | 5323 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-server hbase-mapreduce U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/5/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] anoopsjohn commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368411019
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -387,4 +409,23 @@ public Comparator getSimpleComparator() {
public Comparator getSimpleComparator() {
return new BBKVComparator(this);
}
+
+ /**
+ * Utility method that makes a guess at comparator to use based off passed tableName.
+ * Use in extreme when no comparator specified.
+ * @return CellComparator to use going off the <code>tableName</code> passed.
+ */
+ public static CellComparator getCellComparator(TableName tableName) {
+ return getCellComparator(tableName.toBytes());
Review comment:
Can we just use TableName.isMetaTableName(TableName) here? Why to have the indirection of toBytes and then compare bytes?
----------------------------------------------------------------
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] anoopsjohn commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368410341
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContext.java
##########
@@ -114,6 +118,11 @@ public HFileContext(HFileContext context) {
this.hfileName = hfileName;
this.columnFamily = columnFamily;
this.tableName = tableName;
+ // If no cellComparator specified, make a guess based off tablename. If hbase:meta, then should
+ // be the meta table comparator. Comparators are per table.
+ this.cellComparator = cellComparator != null? cellComparator:
+ this.tableName != null? CellComparatorImpl.getCellComparator(this.tableName):
Review comment:
This I like :-) Our best effort to make things work.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r369150113
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -377,6 +378,27 @@ private static int compareRows(byte[] left, int loffset, int llength, byte[] rig
return result;
}
+ @Override
+ public int compareRows(ByteBuffer row, Cell cell) {
+ byte [] array;
+ int offset;
+ int len = row.remaining();
+ if (row.hasArray()) {
+ array = row.array();
+ offset = row.position() + row.arrayOffset();
+ } else {
+ // This is awful, we copy the row array if offheap just so we can do a compare.
+ // We do this elsewhere too when Cell is backed by an offheap ByteBuffer.
+ // Needs fixing. TODO.
Review comment:
Made the comment more mild. Added pointer to BBUtils, that we do here what it does. Added note to fix ever having to copy on heap.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368612232
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/FixedFileTrailer.java
##########
@@ -624,19 +624,22 @@ private String getHBase1CompatibleName(final String comparator) {
return comparatorKlass;
}
- public static CellComparator createComparator(
- String comparatorClassName) throws IOException {
+ static CellComparator createComparator(String comparatorClassName) throws IOException {
+ if (comparatorClassName.equals(CellComparatorImpl.COMPARATOR.getClass().getName())) {
+ return CellComparatorImpl.COMPARATOR;
+ } else if (comparatorClassName.equals(
+ CellComparatorImpl.META_COMPARATOR.getClass().getName())) {
+ return CellComparatorImpl.META_COMPARATOR;
+ }
Review comment:
Just to avoid the reflection in near all cases.
----------------------------------------------------------------
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 #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-576986412
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 3m 17s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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 20 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 52s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 6m 52s | master passed |
| +1 :green_heart: | compile | 2m 0s | master passed |
| +1 :green_heart: | checkstyle | 2m 11s | master passed |
| +1 :green_heart: | shadedjars | 5m 26s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 24s | master passed |
| +0 :ok: | spotbugs | 5m 14s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 7m 6s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 44s | the patch passed |
| +1 :green_heart: | compile | 1m 50s | the patch passed |
| +1 :green_heart: | javac | 1m 50s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 24s | hbase-common: The patch generated 0 new + 17 unchanged - 32 fixed = 17 total (was 49) |
| -1 :x: | checkstyle | 1m 12s | hbase-server: The patch generated 1 new + 54 unchanged - 156 fixed = 55 total (was 210) |
| -1 :x: | checkstyle | 0m 18s | hbase-mapreduce: The patch generated 1 new + 0 unchanged - 18 fixed = 1 total (was 18) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 0s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 18s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 12s | the patch passed |
| +1 :green_heart: | findbugs | 6m 56s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 10s | hbase-common in the patch passed. |
| -1 :x: | unit | 33m 7s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 19m 20s | hbase-mapreduce in the patch passed. |
| +1 :green_heart: | asflicense | 0m 54s | The patch does not generate ASF License warnings. |
| | | 133m 27s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.regionserver.compactions.TestStripeCompactionPolicy |
| | hadoop.hbase.regionserver.compactions.TestDateTieredCompactor |
| | hadoop.hbase.regionserver.compactions.TestStripeCompactor |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1062 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux b5eec9873b25 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-1062/out/precommit/personality/provided.sh |
| git revision | master / ba3463d9de |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/3/artifact/out/diff-checkstyle-hbase-server.txt |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/3/artifact/out/diff-checkstyle-hbase-mapreduce.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/3/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/3/testReport/ |
| Max. process+thread count | 5347 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-server hbase-mapreduce U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/3/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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368611582
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -387,4 +409,23 @@ public Comparator getSimpleComparator() {
public Comparator getSimpleComparator() {
return new BBKVComparator(this);
}
+
+ /**
+ * Utility method that makes a guess at comparator to use based off passed tableName.
+ * Use in extreme when no comparator specified.
+ * @return CellComparator to use going off the <code>tableName</code> passed.
+ */
+ public static CellComparator getCellComparator(TableName tableName) {
+ return getCellComparator(tableName.toBytes());
Review comment:
Need both. In actual filecontext, it hosts tablename as bytes only -- not as a TableName object. The TableName.toBytes doesn't actually make bytes. TN itself hosts the name in bytes.
Maybe I should be clearer in a comment that no new arrays are being made in this code?
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368613369
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
##########
@@ -319,7 +312,7 @@ public Writer create() throws IOException {
LOG.debug("Unable to set drop behind on {}", path.getName());
}
}
- return new HFileWriterImpl(conf, cacheConf, path, ostream, comparator, fileContext);
+ return new HFileWriterImpl(conf, cacheConf, path, ostream, fileContext);
Review comment:
Yes sir. Idea is that there is one place to get comparator to use. Before this patch, comparator was passed down most of the time but in encoder context and elsewhere in a few locations, comparator was hardcoded because not passed from above.
----------------------------------------------------------------
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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-575812631
Patch looks big but core changes are small just having cellcomparator in context only and not all over the place.
----------------------------------------------------------------
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 #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-575951082
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 17s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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 20 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 50s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 7m 9s | master passed |
| +1 :green_heart: | compile | 2m 22s | master passed |
| +1 :green_heart: | checkstyle | 2m 34s | master passed |
| +1 :green_heart: | shadedjars | 6m 25s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 50s | master passed |
| +0 :ok: | spotbugs | 5m 51s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 7m 48s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 7m 31s | the patch passed |
| +1 :green_heart: | compile | 2m 13s | the patch passed |
| +1 :green_heart: | javac | 2m 13s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 29s | hbase-common: The patch generated 0 new + 14 unchanged - 18 fixed = 14 total (was 32) |
| +1 :green_heart: | checkstyle | 1m 28s | hbase-server: The patch generated 0 new + 206 unchanged - 4 fixed = 206 total (was 210) |
| +1 :green_heart: | checkstyle | 0m 25s | hbase-mapreduce: The patch generated 0 new + 17 unchanged - 1 fixed = 17 total (was 18) |
| +1 :green_heart: | whitespace | 0m 1s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 6m 10s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 22m 56s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 27s | the patch passed |
| +1 :green_heart: | findbugs | 8m 43s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 42s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 174m 35s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 17m 3s | hbase-mapreduce in the patch passed. |
| +1 :green_heart: | asflicense | 1m 29s | The patch does not generate ASF License warnings. |
| | | 289m 41s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1062 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux df395f59a8f4 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-1062/out/precommit/personality/provided.sh |
| git revision | master / 167892ce64 |
| Default Java | 1.8.0_181 |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/4/testReport/ |
| Max. process+thread count | 5308 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-server hbase-mapreduce U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/4/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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368310922
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java
##########
@@ -276,8 +275,8 @@ private Path writeStoreFile() throws IOException {
Path storeFileParentDir = new Path(TEST_UTIL.getDataTestDir(), "TestHFile");
HFileContext meta = new HFileContextBuilder().withBlockSize(64 * 1024).build();
StoreFileWriter sfw =
- new StoreFileWriter.Builder(conf, fs).withOutputDir(storeFileParentDir)
- .withComparator(CellComparatorImpl.COMPARATOR).withFileContext(meta).build();
+ new StoreFileWriter.Builder(conf, fs).withOutputDir(storeFileParentDir).
+ withFileContext(meta).build();
Review comment:
NIT: Most of the time we use a leading `.`.
----------------------------------------------------------------
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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368310754
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
##########
@@ -1827,7 +1827,8 @@ protected HFileBlock readBlockDataInternal(FSDataInputStream is, long offset,
@Override
public void setIncludesMemStoreTS(boolean includesMemstoreTS) {
- this.fileContext.setIncludesMvcc(includesMemstoreTS);
+ this.fileContext = new HFileContextBuilder(this.fileContext).
+ withIncludesMvcc(includesMemstoreTS).build();
Review comment:
NIT: Most of the time we use a leading `.`.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368610567
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexSeekerV1.java
##########
@@ -154,19 +153,6 @@ private int binarySearch(Cell seekCell, boolean seekBefore) {
}
}
- private int compareRows(ByteBuffer row, Cell seekCell) {
Review comment:
We can't have encoders doing their own logic figuring Cell Compare else the issue this patch fixes will perpetually haunt us as encoding implementors make the same mistake of around comparators over and over again; i.e. presuming only one comparator in the system or knowing there are two at least but defaulting to user-space comparator since that is what is used 99.9% of the time.
CellComparator already has methods that do row compare. This is just adding one that hosts the row in ByteBuffer instead of an array... an override.
CellComparators need to do more ByteBuffering, not less. CellComparator needs to learn how to do compares w/o making copies of the ByteBuffer content as it currently does in BBUtills if the BB doesn't have an array because comparators don't know how to stride through a ByteBuffer.
bq. Also within this context we know the passed row BB is sliced for the rk bytes alone. Within CellComparator we can not have such assumption.
CellComparator has one method that does this already taking an array.
----------------------------------------------------------------
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 #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-575930815
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 32s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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 1s | The patch appears to include 23 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 38s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 9s | master passed |
| +1 :green_heart: | compile | 1m 49s | master passed |
| +1 :green_heart: | checkstyle | 2m 6s | master passed |
| +1 :green_heart: | shadedjars | 4m 42s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 19s | master passed |
| +0 :ok: | spotbugs | 4m 33s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 7s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 57s | the patch passed |
| +1 :green_heart: | compile | 1m 48s | the patch passed |
| +1 :green_heart: | javac | 1m 48s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 24s | hbase-common: The patch generated 0 new + 14 unchanged - 18 fixed = 14 total (was 32) |
| +1 :green_heart: | checkstyle | 1m 17s | hbase-server: The patch generated 0 new + 465 unchanged - 5 fixed = 465 total (was 470) |
| +1 :green_heart: | checkstyle | 0m 20s | hbase-mapreduce: The patch generated 0 new + 17 unchanged - 1 fixed = 17 total (was 18) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 4m 39s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 15m 47s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 18s | the patch passed |
| +1 :green_heart: | findbugs | 6m 26s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 19s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 157m 36s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 15m 34s | hbase-mapreduce in the patch passed. |
| +1 :green_heart: | asflicense | 1m 48s | The patch does not generate ASF License warnings. |
| | | 244m 57s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1062 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 39f5f01fc846 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1062/out/precommit/personality/provided.sh |
| git revision | master / 167892ce64 |
| Default Java | 1.8.0_181 |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/3/testReport/ |
| Max. process+thread count | 5563 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-server hbase-mapreduce U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/3/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] Apache-HBase commented on issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-575841816
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 33s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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 18 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 46s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 7m 51s | master passed |
| +1 :green_heart: | compile | 1m 56s | master passed |
| +1 :green_heart: | checkstyle | 2m 28s | master passed |
| +1 :green_heart: | shadedjars | 5m 9s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 15s | master passed |
| +0 :ok: | spotbugs | 4m 57s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 35s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 33s | the patch passed |
| +1 :green_heart: | compile | 1m 48s | the patch passed |
| +1 :green_heart: | javac | 1m 48s | the patch passed |
| -1 :x: | checkstyle | 0m 26s | hbase-common: The patch generated 2 new + 14 unchanged - 18 fixed = 16 total (was 32) |
| -1 :x: | checkstyle | 1m 38s | hbase-server: The patch generated 3 new + 443 unchanged - 5 fixed = 446 total (was 448) |
| +1 :green_heart: | checkstyle | 0m 20s | hbase-mapreduce: The patch generated 0 new + 17 unchanged - 1 fixed = 17 total (was 18) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 3s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 22s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 16s | the patch passed |
| -1 :x: | findbugs | 0m 58s | hbase-common generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 9s | hbase-common in the patch passed. |
| -1 :x: | unit | 32m 36s | hbase-server in the patch failed. |
| -1 :x: | unit | 16m 4s | hbase-mapreduce in the patch failed. |
| +1 :green_heart: | asflicense | 0m 58s | The patch does not generate ASF License warnings. |
| | | 129m 51s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:hbase-common |
| | Using .equals to compare two byte[]'s, (equivalent to ==) in org.apache.hadoop.hbase.CellComparatorImpl.getCellComparator(byte[]) At CellComparatorImpl.java:byte[]'s, (equivalent to ==) in org.apache.hadoop.hbase.CellComparatorImpl.getCellComparator(byte[]) At CellComparatorImpl.java:[line 428] |
| Failed junit tests | hadoop.hbase.regionserver.TestBulkLoad |
| | hadoop.hbase.io.hfile.TestReseekTo |
| | hadoop.hbase.io.hfile.TestFixedFileTrailer |
| | hadoop.hbase.regionserver.TestHStoreFile |
| | hadoop.hbase.mapreduce.TestHFileOutputFormat2 |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1062 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux ec3cc2104878 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-1062/out/precommit/personality/provided.sh |
| git revision | master / df8f80a819 |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/artifact/out/diff-checkstyle-hbase-common.txt |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/artifact/out/diff-checkstyle-hbase-server.txt |
| findbugs | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/artifact/out/new-findbugs-hbase-common.html |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/artifact/out/patch-unit-hbase-server.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/artifact/out/patch-unit-hbase-mapreduce.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/testReport/ |
| Max. process+thread count | 5260 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-server hbase-mapreduce U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-577259480
Checkstyle failure is for the method that is too long (expected). The failed test passes locally. Let me rerun to make sure.
----------------------------------------------------------------
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] ramkrish86 commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
ramkrish86 commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368443164
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java
##########
@@ -135,14 +132,16 @@ private StoreFileWriter(FileSystem fs, Path path, final Configuration conf, Cach
// init bloom context
switch (bloomType) {
case ROW:
- bloomContext = new RowBloomContext(generalBloomFilterWriter, comparator);
+ bloomContext =
+ new RowBloomContext(generalBloomFilterWriter, fileContext.getCellComparator());
break;
case ROWCOL:
- bloomContext = new RowColBloomContext(generalBloomFilterWriter, comparator);
+ bloomContext =
+ new RowColBloomContext(generalBloomFilterWriter, fileContext.getCellComparator());
Review comment:
So every where it comes from the context only including for the blooms
----------------------------------------------------------------
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] anoopsjohn commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368405196
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java
##########
@@ -80,6 +83,24 @@ static CellComparator getInstance() {
*/
int compareRows(Cell cell, byte[] bytes, int offset, int length);
+ /**
+ * @param row ByteBuffer that wraps a row; will read from current position and will reading all
+ * remaining; will not disturb the ByteBuffer internal state.
+ * @return greater than 0 if leftCell is bigger, less than 0 if rightCell is bigger, 0 if both
+ * cells are equal
+ */
+ default int compareRows(ByteBuffer row, Cell cell) {
Review comment:
The BB passed here contain only the row bytes? The BB is sliced for row alone?
----------------------------------------------------------------
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 #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-576918318
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 12s | 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 20 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 33s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 43s | master passed |
| +1 :green_heart: | compile | 1m 47s | master passed |
| +1 :green_heart: | checkstyle | 2m 1s | master passed |
| +1 :green_heart: | shadedjars | 5m 16s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 20s | master passed |
| +0 :ok: | spotbugs | 5m 20s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 7m 5s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 49s | the patch passed |
| +1 :green_heart: | compile | 2m 0s | the patch passed |
| +1 :green_heart: | javac | 2m 0s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 26s | hbase-common: The patch generated 0 new + 14 unchanged - 18 fixed = 14 total (was 32) |
| +1 :green_heart: | checkstyle | 1m 20s | hbase-server: The patch generated 0 new + 206 unchanged - 4 fixed = 206 total (was 210) |
| +1 :green_heart: | checkstyle | 0m 20s | hbase-mapreduce: The patch generated 0 new + 17 unchanged - 1 fixed = 17 total (was 18) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 11s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 59s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 16s | the patch passed |
| +1 :green_heart: | findbugs | 7m 28s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 13s | hbase-common in the patch passed. |
| -1 :x: | unit | 201m 20s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 23m 31s | hbase-mapreduce in the patch passed. |
| +1 :green_heart: | asflicense | 2m 32s | The patch does not generate ASF License warnings. |
| | | 305m 27s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.replication.TestReplicationDroppedTables |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1062 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 887d3a3fdc5d 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-1062/out/precommit/personality/provided.sh |
| git revision | master / 569ac1232a |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/1/testReport/ |
| Max. process+thread count | 5421 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-server hbase-mapreduce U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-576048953
Thank you for the review @HorizonNet Will put up a patch in a while that implements your nits.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368615350
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java
##########
@@ -135,14 +132,16 @@ private StoreFileWriter(FileSystem fs, Path path, final Configuration conf, Cach
// init bloom context
switch (bloomType) {
case ROW:
- bloomContext = new RowBloomContext(generalBloomFilterWriter, comparator);
+ bloomContext =
+ new RowBloomContext(generalBloomFilterWriter, fileContext.getCellComparator());
break;
case ROWCOL:
- bloomContext = new RowColBloomContext(generalBloomFilterWriter, comparator);
+ bloomContext =
+ new RowColBloomContext(generalBloomFilterWriter, fileContext.getCellComparator());
Review comment:
Yes sir. One place only instead of hardcodings and guesses at which to use.
----------------------------------------------------------------
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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-576339559
@ramkrish86 and @anoopsjohn Thanks for the reviews. Helps. You object to the reshuffle suggesting that the encoder keep its comparator manipulations local. I tried that initially but when it meant trying to figure which cellcomparator when, and then casting the comparator and trying to figure how to do meta cell comparator when no current support, and then the thought that any encoder that needs to change dependent on the table they are running against would have to do the same thing, it made me take pause and do this more fundamental change.
Let me know if my argument does not convince and I'll just hack up something that works locally for this one encoder. Thanks.
----------------------------------------------------------------
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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368310276
##########
File path: hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat2.java
##########
@@ -424,15 +423,14 @@ private WriterLength getNewWriter(byte[] tableName, byte[] family, Configuration
HFileContext hFileContext = contextBuilder.build();
if (null == favoredNodes) {
wl.writer =
- new StoreFileWriter.Builder(conf, CacheConfig.DISABLED, fs)
- .withOutputDir(familydir).withBloomType(bloomType)
- .withComparator(CellComparator.getInstance()).withFileContext(hFileContext).build();
+ new StoreFileWriter.Builder(conf, CacheConfig.DISABLED, fs).
+ withOutputDir(familydir).withBloomType(bloomType).
+ withFileContext(hFileContext).build();
} else {
wl.writer =
- new StoreFileWriter.Builder(conf, CacheConfig.DISABLED, new HFileSystem(fs))
- .withOutputDir(familydir).withBloomType(bloomType)
- .withComparator(CellComparator.getInstance()).withFileContext(hFileContext)
- .withFavoredNodes(favoredNodes).build();
+ new StoreFileWriter.Builder(conf, CacheConfig.DISABLED, new HFileSystem(fs)).
+ withOutputDir(familydir).withBloomType(bloomType).
Review comment:
NIT: Most of the time we use a leading `.`.
----------------------------------------------------------------
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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368309845
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -377,6 +378,27 @@ private static int compareRows(byte[] left, int loffset, int llength, byte[] rig
return result;
}
+ @Override
+ public int compareRows(ByteBuffer row, Cell cell) {
+ byte [] array;
+ int offset;
+ int len = row.remaining();
+ if (row.hasArray()) {
+ array = row.array();
+ offset = row.position() + row.arrayOffset();
+ } else {
+ // This is awful, we copy the row array if offheap just so we can do a compare.
+ // We do this elsewhere too when Cell is backed by an offheap ByteBuffer.
+ // Needs fixing. TODO.
Review comment:
Is there already a Jira for this?
----------------------------------------------------------------
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 #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-576956704
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 36s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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 20 new or modified test files. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 44s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 8m 10s | master passed |
| +1 :green_heart: | compile | 2m 24s | master passed |
| +1 :green_heart: | checkstyle | 2m 17s | master passed |
| +1 :green_heart: | shadedjars | 5m 9s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 15s | master passed |
| +0 :ok: | spotbugs | 5m 4s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 39s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 32s | the patch passed |
| +1 :green_heart: | compile | 1m 46s | the patch passed |
| +1 :green_heart: | javac | 1m 46s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 24s | hbase-common: The patch generated 0 new + 17 unchanged - 32 fixed = 17 total (was 49) |
| -1 :x: | checkstyle | 1m 14s | hbase-server: The patch generated 1 new + 54 unchanged - 156 fixed = 55 total (was 210) |
| -1 :x: | checkstyle | 0m 18s | hbase-mapreduce: The patch generated 1 new + 0 unchanged - 18 fixed = 1 total (was 18) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 3s | 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 | 1m 13s | the patch passed |
| +1 :green_heart: | findbugs | 6m 55s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 9s | hbase-common in the patch passed. |
| -1 :x: | unit | 32m 25s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 18m 34s | hbase-mapreduce in the patch passed. |
| +1 :green_heart: | asflicense | 1m 1s | The patch does not generate ASF License warnings. |
| | | 132m 9s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.regionserver.compactions.TestStripeCompactionPolicy |
| | hadoop.hbase.regionserver.compactions.TestDateTieredCompactor |
| | hadoop.hbase.regionserver.compactions.TestStripeCompactor |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1062 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux e3c868edfbbf 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-1062/out/precommit/personality/provided.sh |
| git revision | master / 00e64d83e8 |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/artifact/out/diff-checkstyle-hbase-server.txt |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/artifact/out/diff-checkstyle-hbase-mapreduce.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/testReport/ |
| Max. process+thread count | 5380 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-server hbase-mapreduce U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1062/2/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] anoopsjohn commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368415152
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
##########
@@ -1827,7 +1827,8 @@ protected HFileBlock readBlockDataInternal(FSDataInputStream is, long offset,
@Override
public void setIncludesMemStoreTS(boolean includesMemstoreTS) {
- this.fileContext.setIncludesMvcc(includesMemstoreTS);
+ this.fileContext = new HFileContextBuilder(this.fileContext).
+ withIncludesMvcc(includesMemstoreTS).build();
Review comment:
Do we really need to build new HFileContextBuilder and HFileContext to set?
----------------------------------------------------------------
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] ramkrish86 commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
ramkrish86 commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368439982
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexSeekerV1.java
##########
@@ -131,8 +131,7 @@ private int binarySearch(Cell seekCell, boolean seekBefore) {
int comp = 0;
while (low <= high) {
mid = low + ((high - low) >> 1);
- ByteBuffer row = getRow(mid);
- comp = compareRows(row, seekCell);
+ comp = this.cellComparator.compareRows(getRow(mid), seekCell);
Review comment:
I too agree to @anoopsjohn . Seems we deliberately left this compareRows() here. Is there a similar compareRows in every encoder?
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368323660
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
##########
@@ -85,36 +89,37 @@
@Test
public void testUsingMetaAndBinary() throws IOException {
- FileSystem filesystem = FileSystem.get(conf);
Path rootdir = UTIL.getDataTestDirOnTestFS();
// Up flush size else we bind up when we use default catalog flush of 16k.
- TableDescriptorBuilder metaBuilder = UTIL.getMetaTableDescriptorBuilder()
- .setMemStoreFlushSize(64 * 1024 * 1024);
-
+ TableDescriptors tds = new FSTableDescriptors(UTIL.getConfiguration());
+ TableDescriptorBuilder metaBuilder = TableDescriptorBuilder.
+ newBuilder(tds.get(TableName.META_TABLE_NAME)).setMemStoreFlushSize(64 * 1024 * 1024);
+ TableDescriptor td = metaBuilder.build();
HRegion mr = HBaseTestingUtility.createRegionAndWAL(HRegionInfo.FIRST_META_REGIONINFO,
- rootdir, this.conf, metaBuilder.build());
+ rootdir, this.conf, td);
try {
// Write rows for three tables 'A', 'B', and 'C'.
for (char c = 'A'; c < 'D'; c++) {
HTableDescriptor htd = new HTableDescriptor(TableName.valueOf("" + c));
final int last = 128;
final int interval = 2;
for (int i = 0; i <= last; i += interval) {
- HRegionInfo hri = new HRegionInfo(htd.getTableName(),
- i == 0 ? HConstants.EMPTY_BYTE_ARRAY : Bytes.toBytes((byte) i),
- i == last ? HConstants.EMPTY_BYTE_ARRAY : Bytes.toBytes((byte) i + interval));
-
+ RegionInfo hri = RegionInfoBuilder.newBuilder(htd.getTableName()).
+ setStartKey(i == 0? HConstants.EMPTY_BYTE_ARRAY: Bytes.toBytes((byte)i)).
Review comment:
Smile. I like the trailing '.'. Makes you keep reading to see what is next. IIRC, I got it from the effective java book. I should see what is in our coding template.
----------------------------------------------------------------
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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368309931
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -387,4 +409,23 @@ public Comparator getSimpleComparator() {
public Comparator getSimpleComparator() {
return new BBKVComparator(this);
}
+
+ /**
+ * Utility method that makes a guess at comparator to use based off passed tableName.
+ * Use in extreme when no comparator specified.
+ * @return CellComparator to use going off the <code>tableName</code> passed.
Review comment:
NIT: Use `{@code tableName}` instead.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368323917
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -377,6 +378,27 @@ private static int compareRows(byte[] left, int loffset, int llength, byte[] rig
return result;
}
+ @Override
+ public int compareRows(ByteBuffer row, Cell cell) {
+ byte [] array;
+ int offset;
+ int len = row.remaining();
+ if (row.hasArray()) {
+ array = row.array();
+ offset = row.position() + row.arrayOffset();
+ } else {
+ // This is awful, we copy the row array if offheap just so we can do a compare.
+ // We do this elsewhere too when Cell is backed by an offheap ByteBuffer.
+ // Needs fixing. TODO.
Review comment:
No.
Since making this comment, I see that we do this whenever the BB is offheap. Let me make an issue. This issue adds a method to CellComparator that takes a row held in a ByteBuffer. We need more of this with methods in Comparator that can stride through a BB by index so we don't have to copy on heap to compare. The hard part is an implementation that does not insist on two compare methods -- one for array and another for BB. Would be good if could have one compare only.
----------------------------------------------------------------
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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-575907442
Fix checkstyle
----------------------------------------------------------------
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 #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack merged pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r369158518
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileInfo.java
##########
@@ -380,7 +386,8 @@ private HFileContext createHFileContext(Path path,
HFileContextBuilder builder = new HFileContextBuilder()
.withHBaseCheckSum(true)
.withHFileName(path.getName())
- .withCompression(trailer.getCompressionCodec());
+ .withCompression(trailer.getCompressionCodec())
+ .withCellComparator(trailer.createComparator(trailer.getComparatorClassName()));
Review comment:
Hmm... Its a static. createComparator is probably right name here...
----------------------------------------------------------------
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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-575909543
Undid mistaken setting of encoding and blooms on meta schema. Thats for HBASE-21065.
----------------------------------------------------------------
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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368309982
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -387,4 +409,23 @@ public Comparator getSimpleComparator() {
public Comparator getSimpleComparator() {
return new BBKVComparator(this);
}
+
+ /**
+ * Utility method that makes a guess at comparator to use based off passed tableName.
+ * Use in extreme when no comparator specified.
+ * @return CellComparator to use going off the <code>tableName</code> passed.
+ */
+ public static CellComparator getCellComparator(TableName tableName) {
+ return getCellComparator(tableName.toBytes());
+ }
+
+ /**
+ * Utility method that makes a guess at comparator to use based off passed tableName.
+ * Use in extreme when no comparator specified.
+ * @return CellComparator to use going off the <code>tableName</code> passed.
Review comment:
NIT: Use `{@code tableName}` instead.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r369150187
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java
##########
@@ -387,4 +409,23 @@ public Comparator getSimpleComparator() {
public Comparator getSimpleComparator() {
return new BBKVComparator(this);
}
+
+ /**
+ * Utility method that makes a guess at comparator to use based off passed tableName.
+ * Use in extreme when no comparator specified.
+ * @return CellComparator to use going off the <code>tableName</code> passed.
Review comment:
Done
----------------------------------------------------------------
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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368310209
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContext.java
##########
@@ -114,6 +118,11 @@ public HFileContext(HFileContext context) {
this.hfileName = hfileName;
this.columnFamily = columnFamily;
this.tableName = tableName;
+ // If no cellComparator specified, make a guess based off tablename. If hbase:meta, then should
+ // be the meta table comparator. Comparators are per table.
+ this.cellComparator = cellComparator != null? cellComparator:
Review comment:
NIT: Whitespace before `?` and `:`.
----------------------------------------------------------------
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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-575965360
Review if anyone has a chance. Thanks.
----------------------------------------------------------------
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] anoopsjohn commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368413543
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
##########
@@ -319,7 +312,7 @@ public Writer create() throws IOException {
LOG.debug("Unable to set drop behind on {}", path.getName());
}
}
- return new HFileWriterImpl(conf, cacheConf, path, ostream, comparator, fileContext);
+ return new HFileWriterImpl(conf, cacheConf, path, ostream, fileContext);
Review comment:
So here the comparator class name will be passed within the fileContext from upper layers. 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 commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r369162290
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileReaderImpl.java
##########
@@ -66,9 +65,8 @@ Path makeNewFile() throws IOException {
HFileContext context =
new HFileContextBuilder().withBlockSize(blocksize).withIncludesTags(true).build();
Configuration conf = TEST_UTIL.getConfiguration();
- HFile.Writer writer =
- HFile.getWriterFactoryNoCache(conf).withOutputStream(fout).withFileContext(context)
- .withComparator(CellComparatorImpl.COMPARATOR).create();
+ HFile.Writer writer = HFile.getWriterFactoryNoCache(conf).
+ withOutputStream(fout).withFileContext(context).create();
Review comment:
I went through the patch and I made all align w/ your suggestions. Thanks @HorizonNet .
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r369154999
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
##########
@@ -361,7 +361,7 @@ static HFileBlock createFromBuff(ByteBuff buf, boolean usesHBaseChecksum, final
// This constructor is called when we deserialize a block from cache and when we read a block in
// from the fs. fileCache is null when deserialized from cache so need to make up one.
HFileContextBuilder fileContextBuilder =
- fileContext != null ? new HFileContextBuilder(fileContext) : new HFileContextBuilder();
+ fileContext != null ? new HFileContextBuilder(fileContext): new HFileContextBuilder();
Review comment:
Done
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368643872
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
##########
@@ -85,36 +89,37 @@
@Test
public void testUsingMetaAndBinary() throws IOException {
- FileSystem filesystem = FileSystem.get(conf);
Path rootdir = UTIL.getDataTestDirOnTestFS();
// Up flush size else we bind up when we use default catalog flush of 16k.
- TableDescriptorBuilder metaBuilder = UTIL.getMetaTableDescriptorBuilder()
- .setMemStoreFlushSize(64 * 1024 * 1024);
-
+ TableDescriptors tds = new FSTableDescriptors(UTIL.getConfiguration());
+ TableDescriptorBuilder metaBuilder = TableDescriptorBuilder.
+ newBuilder(tds.get(TableName.META_TABLE_NAME)).setMemStoreFlushSize(64 * 1024 * 1024);
+ TableDescriptor td = metaBuilder.build();
HRegion mr = HBaseTestingUtility.createRegionAndWAL(HRegionInfo.FIRST_META_REGIONINFO,
- rootdir, this.conf, metaBuilder.build());
+ rootdir, this.conf, td);
try {
// Write rows for three tables 'A', 'B', and 'C'.
for (char c = 'A'; c < 'D'; c++) {
HTableDescriptor htd = new HTableDescriptor(TableName.valueOf("" + c));
final int last = 128;
final int interval = 2;
for (int i = 0; i <= last; i += interval) {
- HRegionInfo hri = new HRegionInfo(htd.getTableName(),
- i == 0 ? HConstants.EMPTY_BYTE_ARRAY : Bytes.toBytes((byte) i),
- i == last ? HConstants.EMPTY_BYTE_ARRAY : Bytes.toBytes((byte) i + interval));
-
+ RegionInfo hri = RegionInfoBuilder.newBuilder(htd.getTableName()).
+ setStartKey(i == 0? HConstants.EMPTY_BYTE_ARRAY: Bytes.toBytes((byte)i)).
Review comment:
Back again...
Let me do as you suggest both here and above where you state 'Most of the time we use a ...'. Onus is on me to put in place checkstyle rules if I want to go against precedent ('Most of the time...'). Thanks for reviews.
----------------------------------------------------------------
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 #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368323705
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
##########
@@ -361,7 +361,7 @@ static HFileBlock createFromBuff(ByteBuff buf, boolean usesHBaseChecksum, final
// This constructor is called when we deserialize a block from cache and when we read a block in
// from the fs. fileCache is null when deserialized from cache so need to make up one.
HFileContextBuilder fileContextBuilder =
- fileContext != null ? new HFileContextBuilder(fileContext) : new HFileContextBuilder();
+ fileContext != null ? new HFileContextBuilder(fileContext): new HFileContextBuilder();
Review comment:
I can set these back.
----------------------------------------------------------------
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] anoopsjohn commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368415764
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileInfo.java
##########
@@ -380,7 +386,8 @@ private HFileContext createHFileContext(Path path,
HFileContextBuilder builder = new HFileContextBuilder()
.withHBaseCheckSum(true)
.withHFileName(path.getName())
- .withCompression(trailer.getCompressionCodec());
+ .withCompression(trailer.getCompressionCodec())
+ .withCellComparator(trailer.createComparator(trailer.getComparatorClassName()));
Review comment:
Better to have a trailer.getComparator() which internally do this create?
----------------------------------------------------------------
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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-575869743
Fix complaints and UTs.
----------------------------------------------------------------
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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-576882122
Address review comments. Ran through files I touched with checkstyle.
Looking for a +1 please. Thanks.
----------------------------------------------------------------
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 issue #1062: HBASE-23705 Add
CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#issuecomment-577042490
The checkstyle complaints are two. One is an old method that is > 150 limit. It is 242 currently (after I tried cutting it down). Leaving it. Not related. Other is an import order I finally figured.
On unit tests, I tried them locally and they fail; a checkstyle change suggesting class be final broke mockito. Put up new patch.
Thanks for +1 @anoopsjohn
----------------------------------------------------------------
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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368310991
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
##########
@@ -85,36 +89,37 @@
@Test
public void testUsingMetaAndBinary() throws IOException {
- FileSystem filesystem = FileSystem.get(conf);
Path rootdir = UTIL.getDataTestDirOnTestFS();
// Up flush size else we bind up when we use default catalog flush of 16k.
- TableDescriptorBuilder metaBuilder = UTIL.getMetaTableDescriptorBuilder()
- .setMemStoreFlushSize(64 * 1024 * 1024);
-
+ TableDescriptors tds = new FSTableDescriptors(UTIL.getConfiguration());
+ TableDescriptorBuilder metaBuilder = TableDescriptorBuilder.
+ newBuilder(tds.get(TableName.META_TABLE_NAME)).setMemStoreFlushSize(64 * 1024 * 1024);
+ TableDescriptor td = metaBuilder.build();
HRegion mr = HBaseTestingUtility.createRegionAndWAL(HRegionInfo.FIRST_META_REGIONINFO,
- rootdir, this.conf, metaBuilder.build());
+ rootdir, this.conf, td);
try {
// Write rows for three tables 'A', 'B', and 'C'.
for (char c = 'A'; c < 'D'; c++) {
HTableDescriptor htd = new HTableDescriptor(TableName.valueOf("" + c));
final int last = 128;
final int interval = 2;
for (int i = 0; i <= last; i += interval) {
- HRegionInfo hri = new HRegionInfo(htd.getTableName(),
- i == 0 ? HConstants.EMPTY_BYTE_ARRAY : Bytes.toBytes((byte) i),
- i == last ? HConstants.EMPTY_BYTE_ARRAY : Bytes.toBytes((byte) i + interval));
-
+ RegionInfo hri = RegionInfoBuilder.newBuilder(htd.getTableName()).
+ setStartKey(i == 0? HConstants.EMPTY_BYTE_ARRAY: Bytes.toBytes((byte)i)).
Review comment:
NIT: Most of the time we use a leading `.`.
----------------------------------------------------------------
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] anoopsjohn commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368409615
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexSeekerV1.java
##########
@@ -154,19 +153,6 @@ private int binarySearch(Cell seekCell, boolean seekBefore) {
}
}
- private int compareRows(ByteBuffer row, Cell seekCell) {
Review comment:
I see. This method is moved to CellComparator. Is that required? This impl here correctly handle things. Within the BBUtils we are handling HBB/DBB. But in this patch, it is trying to handle in CellComparator which is not good IMHO.
Also within this context we know the passed row BB is sliced for the rk bytes alone. Within CellComparator we can not have such assumption. So its better to keep it here only. I believe only this class is having such need for compares. Any other place?
----------------------------------------------------------------
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] HorizonNet commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368310934
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileReaderImpl.java
##########
@@ -66,9 +65,8 @@ Path makeNewFile() throws IOException {
HFileContext context =
new HFileContextBuilder().withBlockSize(blocksize).withIncludesTags(true).build();
Configuration conf = TEST_UTIL.getConfiguration();
- HFile.Writer writer =
- HFile.getWriterFactoryNoCache(conf).withOutputStream(fout).withFileContext(context)
- .withComparator(CellComparatorImpl.COMPARATOR).create();
+ HFile.Writer writer = HFile.getWriterFactoryNoCache(conf).
+ withOutputStream(fout).withFileContext(context).create();
Review comment:
NIT: Most of the time we use a leading `.`.
----------------------------------------------------------------
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] ramkrish86 commented on a change in pull request #1062:
HBASE-23705 Add CellComparator to HFileContext
Posted by GitBox <gi...@apache.org>.
ramkrish86 commented on a change in pull request #1062: HBASE-23705 Add CellComparator to HFileContext
URL: https://github.com/apache/hbase/pull/1062#discussion_r368440549
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/FixedFileTrailer.java
##########
@@ -624,19 +624,22 @@ private String getHBase1CompatibleName(final String comparator) {
return comparatorKlass;
}
- public static CellComparator createComparator(
- String comparatorClassName) throws IOException {
+ static CellComparator createComparator(String comparatorClassName) throws IOException {
+ if (comparatorClassName.equals(CellComparatorImpl.COMPARATOR.getClass().getName())) {
+ return CellComparatorImpl.COMPARATOR;
+ } else if (comparatorClassName.equals(
+ CellComparatorImpl.META_COMPARATOR.getClass().getName())) {
+ return CellComparatorImpl.META_COMPARATOR;
+ }
Review comment:
I think it is to avoid the reflections call if possible
----------------------------------------------------------------
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