Skip to content

HBASE-27590 Change Iterable to List in SnapshotFileCache - #4995

Merged
petersomogyi merged 3 commits into
apache:masterfrom
petersomogyi:HBASE-27590
Feb 9, 2023
Merged

HBASE-27590 Change Iterable to List in SnapshotFileCache#4995
petersomogyi merged 3 commits into
apache:masterfrom
petersomogyi:HBASE-27590

Conversation

@petersomogyi

Copy link
Copy Markdown
Contributor

No description provided.

@Apache-HBase

This comment was marked as outdated.

@Apache9

Copy link
Copy Markdown
Contributor

S3 is not the only HFile storage we support, so I think we should also consider the performance impact on HDFS, at least. And better start a discussion thread on the dev list to gain more feedbacks.

@Apache-HBase

This comment was marked as outdated.

@petersomogyi

Copy link
Copy Markdown
ContributorAuthor

S3 is not the only HFile storage we support, so I think we should also consider the performance impact on HDFS, at least. And better start a discussion thread on the dev list to gain more feedbacks.

Let me do a quick test on HDFS if it impacts the performance and after I'll send a mail to the dev list.

@Apache-HBase

This comment was marked as outdated.

@taklwutaklwu left a comment

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.

LGTM

agreed with the point that Duo said about other storage, and thanks for testing HDFS and mentioned the performance difference.

let's see how the discussion goes.

@jojochuangjojochuang left a comment

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.

Oh i see. It's because guava's Iteratorable APIs are lazy:

https://guava.dev/releases/21.0/api/docs/com/google/common/collect/Iterables.html

Performance notes: Unless otherwise noted, all of the iterables produced in this class are lazy, which means that their iterators only advance the backing iteration when absolutely necessary.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache9

Copy link
Copy Markdown
Contributor

So the actual problem here is lazy evaluation which will delay the actual io operation under lock?

Let me take a look at the PR about why lazy evaluation produce bad result here...

@Apache9

Copy link
Copy Markdown
Contributor

As replied on the dev mailing list, I do not think we need to do a big refactoring, just change the way we call SnapshotFileCache.getUnreferencedFiles is enough, where we convert the Iterable to List and use the List to call SnapshotFileCache.getUnreferencedFiles.

Thanks.

@petersomogyi

Copy link
Copy Markdown
ContributorAuthor

As replied on the dev mailing list, I do not think we need to do a big refactoring, just change the way we call SnapshotFileCache.getUnreferencedFiles is enough, where we convert the Iterable to List and use the List to call SnapshotFileCache.getUnreferencedFiles.

Thanks.

Thanks for the suggestion @Apache9! I've tested with this minor change and the improvement was the same.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

@petersomogyipetersomogyi changed the title HBASE-27590 Change Iterable to List in CleanerChoreHBASE-27590 Change Iterable to List in SnapshotFileCacheFeb 8, 2023
@Apache-HBase

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 21sDocker 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 _
+1 💚mvninstall3m 20smaster passed
+1 💚compile2m 22smaster passed
+1 💚checkstyle0m 36smaster passed
+1 💚spotless0m 42sbranch has no errors when running spotless:check.
+1 💚spotbugs1m 25smaster passed
_ Patch Compile Tests _
+1 💚mvninstall3m 19sthe patch passed
+1 💚compile2m 19sthe patch passed
+1 💚javac2m 19sthe patch passed
+1 💚checkstyle0m 32sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck12m 21sPatch does not cause any errors with Hadoop 3.2.4 3.3.4.
+1 💚spotless0m 40spatch has no errors when running spotless:check.
+1 💚spotbugs1m 34sthe patch passed
_ Other Tests _
+1 💚asflicense0m 10sThe patch does not generate ASF License warnings.
37m 3s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4995/5/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#4995
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile
unameLinux c47ae7d64385 5.4.0-135-generic #152-Ubuntu SMP Wed Nov 23 20:19:22 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 6a34aa8
Default JavaEclipse Adoptium-11.0.17+8
Max. process+thread count85 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4995/5/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

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 21sDocker 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 _
+1 💚mvninstall2m 48smaster passed
+1 💚compile0m 40smaster passed
+1 💚shadedjars4m 16sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 24smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 46sthe patch passed
+1 💚compile0m 39sthe patch passed
+1 💚javac0m 38sthe patch passed
+1 💚shadedjars4m 17spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 23sthe patch passed
_ Other Tests _
+1 💚unit205m 29shbase-server in the patch passed.
226m 17s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4995/5/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR#4995
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 617338fc808f 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 / 6a34aa8
Default JavaTemurin-1.8.0_352-b08
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4995/5/testReport/
Max. process+thread count2637 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4995/5/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

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 1sDocker mode activated.
-0 ⚠️yetus0m 4sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚mvninstall3m 18smaster passed
+1 💚compile0m 51smaster passed
+1 💚shadedjars4m 24sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 25smaster passed
_ Patch Compile Tests _
+1 💚mvninstall3m 21sthe patch passed
+1 💚compile0m 47sthe patch passed
+1 💚javac0m 47sthe patch passed
+1 💚shadedjars4m 23spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 24sthe patch passed
_ Other Tests _
-1 ❌unit211m 11shbase-server in the patch failed.
234m 26s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4995/5/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR#4995
Optional Testsjavac javadoc unit shadedjars compile
unameLinux a7ec2888340d 5.4.0-135-generic #152-Ubuntu SMP Wed Nov 23 20:19:22 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 6a34aa8
Default JavaEclipse Adoptium-11.0.17+8
unithttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4995/5/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4995/5/testReport/
Max. process+thread count2487 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4995/5/console
versionsgit=2.34.1 maven=3.8.6
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@petersomogyi

Copy link
Copy Markdown
ContributorAuthor

@Apache9 Please take a look. Thanks.

@petersomogyi
petersomogyi merged commit d2c5af1 into apache:masterFeb 9, 2023
@petersomogyi
petersomogyi deleted the HBASE-27590 branch February 9, 2023 10:20
petersomogyi added a commit that referenced this pull request Feb 9, 2023
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit d2c5af1)
petersomogyi added a commit that referenced this pull request Feb 9, 2023
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit d2c5af1)
petersomogyi added a commit that referenced this pull request Feb 13, 2023
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit d2c5af1)
vinayakphegde pushed a commit to vinayakphegde/hbase that referenced this pull request Apr 4, 2024
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit d2c5af1)
(cherry picked from commit 6c5c65a)
Change-Id: I6f2eed8181fa56024c8d18899d8f6668ec6a36a8
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@petersomogyi@Apache-HBase@Apache9@jojochuang@taklwu