Skip to content

HBASE-27788 Skip family comparing when compare cells inner the store - #5171

Merged
bsglz merged 11 commits into
apache:masterfrom
bsglz:HBASE-27788
May 10, 2023
Merged

HBASE-27788 Skip family comparing when compare cells inner the store#5171
bsglz merged 11 commits into
apache:masterfrom
bsglz:HBASE-27788

Conversation

@bsglz

@bsglzbsglz commented Apr 11, 2023

Copy link
Copy Markdown
Contributor

No description provided.

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache9

Apache9 commented Apr 11, 2023

Copy link
Copy Markdown
Contributor

I used to consider the same but soon I found that there are some tricks in our implementation, as we may generate some fake cell for seeking.

I saw that you have already handled the case where left family length or right family length are zero, this is for some key only cell, but I'm afraid there could still be other type of fake cells, for example, when querying on a bloom filter enabled region, we may use bloom filter to generate a fake cell, if the type of the bloom filter is ROWCOL, maybe we could also include a fake family?

@bsglz

bsglz commented Apr 11, 2023

Copy link
Copy Markdown
ContributorAuthor

I used to consider the same but soon I found that there are some tricks in our implementation, as we may generate some fake cell for seeking.

I saw that you have already handled the case where left family length or right family length are zero, this is for some key only cell, but I'm afraid there could still be other type of fake cells, for example, when querying on a bloom filter enabled region, we may use bloom filter to generate a fake cell, if the type of the bloom filter is ROWCOL, maybe we could also include a fake family?

Yeah, this is a fairly important class and does require detailed testing.
Let me see the case you mentioned first.

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@bsglz

bsglz commented Apr 11, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@Apache9 Checked the code, the bloom filter use empty family both in storing and checking, so seems ok.

 case ROWCOL:
if (!scan.isGetScan()) {
return true;
}
if (columns != null && columns.size() == 1) {
byte[] column = columns.first();
// create the required fake key
Cell kvKey = PrivateCellUtil.createFirstOnRow(row, HConstants.EMPTY_BYTE_ARRAY, column);
return passesGeneralRowColBloomFilter(kvKey);
}
// For multi-column queries the Bloom filter is checked from the
// seekExact operation.
return true;

/**
* An hash key for ROWCOL bloom. This assumes the cells to be serialized in the Keyvalue
* serialization format with Empty column family. Note that the byte representing the family length
* is considered to be 0
*/
@InterfaceAudience.Private
public class RowColBloomHashKey extends CellHashKey {

BTW, the method of checkGeneralBloomFilter use CellComparator.getInstance() directly which return CellComparatorImpl, will change them later.

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
Apache9
Apache9 reviewed Apr 12, 2023
View reviewed changes
Comment threadhbase-common/src/main/java/org/apache/hadoop/hbase/CellComparatorImpl.java Outdated
Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreContext.java Outdated
if (comparator == MetaCellComparator.META_COMPARATOR) {
this.comparator = comparator;
} else {
this.comparator = InnerStoreCellComparator.INNER_STORE_COMPARATOR;

@Apache9Apache9Apr 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit strange, as we may not use the comparator passed in. We should use a new pattern to set the comparator here, otherwise it will confuse developers...

@bsglzbsglzApr 12, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, will refactor here.
Thanks.

@Apache-HBase

This comment has been minimized.

Sign in to view
@bsglz

bsglz commented Apr 12, 2023

Copy link
Copy Markdown
ContributorAuthor

Writing a perf test class for cellComparator, don't merge yet even if the tests pass.

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@bsglz

bsglz commented Apr 12, 2023

Copy link
Copy Markdown
ContributorAuthor

Added perf test class named PerfTestCellComparator.
Below is once running result:

<style type='text/css'></style>
leftFamLenrightFamLencomparatorcost(ms)
00CellComparatorImpl12738
00InnerStoreCellComparator12548
04CellComparatorImpl8541
04InnerStoreCellComparator8142
40CellComparatorImpl8435
40InnerStoreCellComparator8327
44CellComparatorImpl13662
44InnerStoreCellComparator11551

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@bsglz
bsglz requested a review from Apache9April 13, 2023 10:56
@Apache-HBase

This comment has been minimized.

Sign in to view
@bsglz

bsglz commented Apr 14, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

More detailed test report, compareCnt is 1 billion.

<style type='text/css'></style>
compareMethodleftFamLenrightFamLencomparatorcost(ms)diff
compareKV00CellComparatorImpl28850
compareKV00InnerStoreCellComparator27478-5.00%
compareKV04CellComparatorImpl19041
compareKV04InnerStoreCellComparator17391-9.00%
compareKV40CellComparatorImpl18988
compareKV40InnerStoreCellComparator17375-8.00%
compareKV44CellComparatorImpl33360
compareKV44InnerStoreCellComparator27083-19.00%
compareBBKV00CellComparatorImpl34014
compareBBKV00InnerStoreCellComparator31660-7.00%
compareBBKV04CellComparatorImpl20780
compareBBKV04InnerStoreCellComparator208470.00%
compareBBKV40CellComparatorImpl23540
compareBBKV40InnerStoreCellComparator21751-8.00%
compareBBKV44CellComparatorImpl40192
compareBBKV44InnerStoreCellComparator31522-22.00%
compareKVVsBBKV00CellComparatorImpl30979
compareKVVsBBKV00InnerStoreCellComparator29827-4.00%
compareKVVsBBKV04CellComparatorImpl21918
compareKVVsBBKV04InnerStoreCellComparator19143-13.00%
compareKVVsBBKV40CellComparatorImpl22605
compareKVVsBBKV40InnerStoreCellComparator20952-7.00%
compareKVVsBBKV44CellComparatorImpl35561
compareKVVsBBKV44InnerStoreCellComparator29150-18.00%

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@bsglz
bsglz requested a review from bbeaudreaultApril 27, 2023 08:01
@bsglz

bsglz commented Apr 28, 2023

Copy link
Copy Markdown
ContributorAuthor

Any other comments? @Apache9@bbeaudreault
Thanks.

Apache9
Apache9 reviewed Apr 28, 2023
View reviewed changes
Comment threadhbase-common/src/main/java/org/apache/hadoop/hbase/InnerStoreCellComparator.java Outdated

public static CellComparator getInnerStoreCellComparator(Configuration conf, byte[] tableName) {
if (
conf != null && conf.getBoolean(HConstants.USE_META_CELL_COMPARATOR,

@Apache9Apache9Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this judgement in upper layer so we do not need to move USE_META_CELL_COMPARATOR to HConstants? It is just for internal use, we'd better not put it into an IA.Public class...

@bsglzbsglzApr 29, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try.

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@bsglz
bsglz requested a review from Apache9May 1, 2023 10:12
Apache9
Apache9 reviewed May 1, 2023
View reviewed changes
Comment threadhbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/PerfTestCellComparator.java Outdated
*/
@Category({ MediumTests.class })
@RunWith(Parameterized.class)
public class PerfTestCellComparator {

@Apache9Apache9May 1, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd better just post the JMH code on the jira issue? Without JMH, a micro bench is not very stable...

@bsglzbsglzMay 2, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/FixedFileTrailer.java
*/
// We could write the actual class name from 2.0 onwards and handle BC
private String comparatorClassName = CellComparator.getInstance().getClass().getName();
private String comparatorClassName =

@Apache9Apache9May 1, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No compatibility issues?

@bsglzbsglzMay 2, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No compatibility issues?

Thought about it and i think so, because we do the conversion when we save and read the hfile, the actual store is the KVComparator, we can't change this since we want hbase1.x to be able to read the files generated in the new version. See the comment of getHBase1CompatibleName for more detail.
Thanks.

@Apache-HBase

Apache-HBase commented May 2, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 0sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
_ master Compile Tests _
+0 🆗mvndep0m 10sMaven dependency ordering for branch
+1 💚mvninstall3m 50smaster passed
+1 💚compile3m 7smaster passed
+1 💚checkstyle0m 49smaster passed
+1 💚spotless0m 43sbranch has no errors when running spotless:check.
+1 💚spotbugs2m 10smaster passed
_ Patch Compile Tests _
+0 🆗mvndep0m 10sMaven dependency ordering for patch
+1 💚mvninstall3m 36sthe patch passed
+1 💚compile3m 2sthe patch passed
+1 💚javac3m 2sthe patch passed
+1 💚checkstyle0m 48sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck13m 1sPatch does not cause any errors with Hadoop 3.2.4 3.3.4.
+1 💚spotless0m 42spatch has no errors when running spotless:check.
+1 💚spotbugs2m 15sthe patch passed
_ Other Tests _
+1 💚asflicense0m 20sThe patch does not generate ASF License warnings.
43m 18s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5171/18/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#5171
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile
unameLinux 8457a4f9a2ea 5.4.0-144-generic #161-Ubuntu SMP Fri Feb 3 14:49:04 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 21d61cf
Default JavaEclipse Adoptium-11.0.17+8
Max. process+thread count86 (vs. ulimit of 30000)
modulesC: hbase-common hbase-server U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5171/18/console
versionsgit=2.34.1 maven=3.8.6 spotbugs=4.7.3
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented May 2, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 24sDocker mode activated.
-0 ⚠️yetus0m 2sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗mvndep0m 18sMaven dependency ordering for branch
+1 💚mvninstall2m 47smaster passed
+1 💚compile0m 49smaster passed
+1 💚shadedjars4m 38sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 33smaster passed
_ Patch Compile Tests _
+0 🆗mvndep0m 11sMaven dependency ordering for patch
+1 💚mvninstall2m 49sthe patch passed
+1 💚compile0m 48sthe patch passed
+1 💚javac0m 48sthe patch passed
+1 💚shadedjars4m 39spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 32sthe patch passed
_ Other Tests _
+1 💚unit1m 41shbase-common in the patch passed.
+1 💚unit208m 33shbase-server in the patch passed.
232m 42s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5171/18/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR#5171
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 03ca98afe57b 5.4.0-1099-aws #107~18.04.1-Ubuntu SMP Fri Mar 17 16:49:05 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 21d61cf
Default JavaTemurin-1.8.0_352-b08
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5171/18/testReport/
Max. process+thread count2849 (vs. ulimit of 30000)
modulesC: hbase-common hbase-server U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5171/18/console
versionsgit=2.34.1 maven=3.8.6
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented May 2, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 48sDocker mode activated.
-0 ⚠️yetus0m 3sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗mvndep0m 19sMaven dependency ordering for branch
+1 💚mvninstall3m 21smaster passed
+1 💚compile1m 5smaster passed
+1 💚shadedjars4m 24sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 40smaster passed
_ Patch Compile Tests _
+0 🆗mvndep0m 13sMaven dependency ordering for patch
+1 💚mvninstall3m 21sthe patch passed
+1 💚compile1m 4sthe patch passed
+1 💚javac1m 4sthe patch passed
+1 💚shadedjars4m 25spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 41sthe patch passed
_ Other Tests _
+1 💚unit2m 6shbase-common in the patch passed.
+1 💚unit212m 44shbase-server in the patch passed.
239m 52s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5171/18/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR#5171
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 342853696fbb 5.4.0-137-generic #154-Ubuntu SMP Thu Jan 5 17:03:22 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 21d61cf
Default JavaEclipse Adoptium-11.0.17+8
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5171/18/testReport/
Max. process+thread count2995 (vs. ulimit of 30000)
modulesC: hbase-common hbase-server U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5171/18/console
versionsgit=2.34.1 maven=3.8.6
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@bsglz
bsglz requested a review from Apache9May 3, 2023 04:55
@bsglz

bsglz commented May 5, 2023

Copy link
Copy Markdown
ContributorAuthor

Any more comments? @Apache9@bbeaudreault

@bsglz
bsglz merged commit 5d82d4f into apache:masterMay 10, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbeaudreaultbbeaudreaultbbeaudreault approved these changes
@Apache9Apache9Apache9 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bsglz@Apache-HBase@Apache9@bbeaudreault