Skip to content

HBASE-27579 CatalogJanitor can cause data loss due to errors during cleanMergeRegion - #4986

Merged
bbeaudreault merged 3 commits into
apache:masterfrom
HubSpot:HBASE-27579
Jan 20, 2023
Merged

HBASE-27579 CatalogJanitor can cause data loss due to errors during cleanMergeRegion#4986
bbeaudreault merged 3 commits into
apache:masterfrom
HubSpot:HBASE-27579

Conversation

@bbeaudreault

@bbeaudreaultbbeaudreault commented Jan 19, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

There already existed a robust reference check method for cleaning splits, checkDaughterInFs. I cleaned that up a little bit and re-use it in cleanMergeRegion. This way both cleaning splits and merges use the same reference checking logic. This should be easier to maintain going forward.

I added tests in TestCatalogJanitor -- there were none for merges, so i added a base case and a failure mode case.

I decided not to modify HRegionFileSystem.openRegionFromFileSystem -- even if I modified that to throw FileNotFoundException, from a defensive coding perspective I'd rather not assume that exception refers to the region dir specifically. This code is highly destructive, so we rely on an explicit exists call and only delete if that successfully returns false within our method.

@Apache-HBase

Apache-HBase commented Jan 19, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 24sDocker 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 💚mvninstall2m 43smaster passed
+1 💚compile2m 26smaster passed
+1 💚checkstyle0m 34smaster passed
+1 💚spotless0m 40sbranch has no errors when running spotless:check.
+1 💚spotbugs1m 26smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 22sthe patch passed
+1 💚compile2m 23sthe patch passed
+1 💚javac2m 23sthe patch passed
-0 ⚠️checkstyle0m 32shbase-server: The patch generated 1 new + 1 unchanged - 0 fixed = 2 total (was 1)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck8m 58sPatch does not cause any errors with Hadoop 3.2.4 3.3.4.
+1 💚spotless0m 39spatch has no errors when running spotless:check.
+1 💚spotbugs1m 31sthe patch passed
_ Other Tests _
+1 💚asflicense0m 9sThe patch does not generate ASF License warnings.
31m 43s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#4986
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile
unameLinux faea28bbf26b 5.4.0-1093-aws #102~18.04.2-Ubuntu SMP Wed Dec 7 00:31:59 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / ad8f28e
Default JavaEclipse Adoptium-11.0.17+8
checkstylehttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt
Max. process+thread count79 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/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.

@virajjasani
virajjasani self-requested a review January 19, 2023 16:37
virajjasani
virajjasani reviewed Jan 19, 2023
View reviewed changes
Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java
Comment on lines +416 to 418
LOG.error("Error trying to determine if region exists, assuming exists and has references",
ioe);
return new Pair<>(Boolean.TRUE, Boolean.TRUE);

@virajjasanivirajjasaniJan 19, 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.

So this is the big difference b/ the two implementations right?

@bbeaudreaultbbeaudreaultJan 19, 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.

Correct. Previously we'd treat this error as "lets assume it doesn't exist and go forward with cleanup". This assumption was inaccurate in at least some failure modes, so unsafe. The new assumption is "we don't know if the region exists, so let's not risk cleaning up". This is safer, but the downside is we might wait longer to GC merged regions. I think that's not a problem at all.

@virajjasanivirajjasaniJan 20, 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 safer, but the downside is we might wait longer to GC merged regions. I think that's not a problem at all.

+1

@Apache-HBase

Apache-HBase commented Jan 19, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 6sDocker 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 💚mvninstall2m 43smaster passed
+1 💚compile0m 47smaster passed
+1 💚shadedjars3m 53sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 27smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 35sthe patch passed
+1 💚compile0m 48sthe patch passed
+1 💚javac0m 48sthe patch passed
+1 💚shadedjars3m 48spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 25sthe patch passed
_ Other Tests _
+1 💚unit213m 59shbase-server in the patch passed.
235m 3s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR#4986
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 6fb9447ef100 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 / ad8f28e
Default JavaEclipse Adoptium-11.0.17+8
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/testReport/
Max. process+thread count2493 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/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 Jan 19, 2023

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 52sDocker 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 36smaster passed
+1 💚compile0m 59smaster passed
+1 💚shadedjars4m 32sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 23smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 2sthe patch passed
+1 💚compile0m 34sthe patch passed
+1 💚javac0m 34sthe patch passed
+1 💚shadedjars4m 0spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 21sthe patch passed
_ Other Tests _
-1 ❌unit273m 11shbase-server in the patch failed.
294m 25s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR#4986
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 8cbf629f8b2a 5.4.0-1088-aws #96~18.04.1-Ubuntu SMP Mon Oct 17 02:57:48 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / ad8f28e
Default JavaTemurin-1.8.0_352-b08
unithttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/testReport/
Max. process+thread count2623 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/1/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 Jan 19, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 51sDocker 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 34smaster passed
+1 💚compile2m 46smaster passed
+1 💚checkstyle0m 35smaster passed
+1 💚spotless0m 47sbranch has no errors when running spotless:check.
+1 💚spotbugs1m 38smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 50sthe patch passed
+1 💚compile2m 42sthe patch passed
+1 💚javac2m 42sthe patch passed
+1 💚checkstyle0m 43sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck11m 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 6sthe patch passed
_ Other Tests _
+1 💚asflicense0m 11sThe patch does not generate ASF License warnings.
37m 11s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#4986
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile
unameLinux f85311caf618 5.4.0-1092-aws #100~18.04.2-Ubuntu SMP Tue Nov 29 08:39:52 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / ad8f28e
Default JavaEclipse Adoptium-11.0.17+8
Max. process+thread count78 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/3/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.

@bbeaudreault

bbeaudreault commented Jan 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks all for the review.

Having trouble with some flaky tests. I'm going to let it run once more to see if we can get a clean build. So far I've run the failing tests manually and they succeed locally.

@Apache-HBase

Apache-HBase commented Jan 20, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec2m 11sDocker 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 33smaster passed
+1 💚compile2m 22smaster passed
+1 💚checkstyle0m 37smaster passed
+1 💚spotless0m 43sbranch has no errors when running spotless:check.
+1 💚spotbugs1m 30smaster passed
_ Patch Compile Tests _
+1 💚mvninstall3m 11sthe patch passed
+1 💚compile2m 22sthe patch passed
+1 💚javac2m 22sthe patch passed
+1 💚checkstyle0m 36sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck12m 13sPatch does not cause any errors with Hadoop 3.2.4 3.3.4.
+1 💚spotless0m 41spatch has no errors when running spotless:check.
+1 💚spotbugs1m 34sthe patch passed
_ Other Tests _
+1 💚asflicense0m 12sThe patch does not generate ASF License warnings.
38m 38s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#4986
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile
unameLinux 8e4b51fd8cd9 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 / 913cf6b
Default JavaEclipse Adoptium-11.0.17+8
Max. process+thread count82 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/4/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 Jan 20, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 38sDocker 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 41smaster passed
+1 💚compile0m 35smaster passed
+1 💚shadedjars4m 31sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 20smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 40sthe patch passed
+1 💚compile0m 35sthe patch passed
+1 💚javac0m 35sthe patch passed
+1 💚shadedjars4m 31spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 21sthe patch passed
_ Other Tests _
+1 💚unit206m 31shbase-server in the patch passed.
227m 50s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR#4986
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 8369092bfc1a 5.4.0-1088-aws #96~18.04.1-Ubuntu SMP Mon Oct 17 02:57:48 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 913cf6b
Default JavaTemurin-1.8.0_352-b08
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/4/testReport/
Max. process+thread count2818 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/4/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 Jan 20, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 7sDocker 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 14smaster passed
+1 💚compile0m 47smaster passed
+1 💚shadedjars4m 24sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 24smaster passed
_ Patch Compile Tests _
+1 💚mvninstall3m 15sthe patch passed
+1 💚compile0m 47sthe patch passed
+1 💚javac0m 47sthe patch passed
+1 💚shadedjars4m 20spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 24sthe patch passed
_ Other Tests _
+1 💚unit213m 49shbase-server in the patch passed.
236m 52s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR#4986
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 0c5a67b10f0f 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 / 913cf6b
Default JavaEclipse Adoptium-11.0.17+8
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/4/testReport/
Max. process+thread count2715 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4986/4/console
versionsgit=2.34.1 maven=3.8.6
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@bbeaudreault
bbeaudreault merged commit cc4fd8b into apache:masterJan 20, 2023
@bbeaudreault
bbeaudreault deleted the HBASE-27579 branch January 20, 2023 17:29
bbeaudreault added a commit that referenced this pull request Jan 20, 2023
@bbeaudreault
HBASE-27579CatalogJanitor can cause data loss due to errors during c…
c6a1722
…leanMergeRegion (#4986)
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
bbeaudreault added a commit that referenced this pull request Jan 20, 2023
@bbeaudreault
HBASE-27579CatalogJanitor can cause data loss due to errors during c…
a7f776f
…leanMergeRegion (#4986)
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
bbeaudreault added a commit that referenced this pull request Jan 20, 2023
@bbeaudreault
HBASE-27579CatalogJanitor can cause data loss due to errors during c…
34a8914
…leanMergeRegion (#4986)
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
bbeaudreault added a commit to HubSpot/hbase that referenced this pull request Jan 20, 2023
@bbeaudreault
HBASE-27579 CatalogJanitor can cause data loss due to errors during c…
b7fb168
…leanMergeRegion (apache#4986)
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
vinayakphegde pushed a commit to vinayakphegde/hbase that referenced this pull request Apr 4, 2024
@bbeaudreault
HBASE-27579 CatalogJanitor can cause data loss due to errors during c…
17424b7
…leanMergeRegion (apache#4986)
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit a7f776f)
Change-Id: I19696f331e463024e4205036c95a09a5aab75816
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@virajjasanivirajjasanivirajjasani approved these changes
@apurtellapurtellapurtell 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.

5 participants

@bbeaudreault@Apache-HBase@apurtell@Apache9@virajjasani