HBASE-23680 RegionProcedureStore missing cleaning of hfile archive by Apache9 · Pull Request #1022 · apache/hbase

Apache9 · 2020-01-11T07:14:57Z

No description provided.

saintstack · 2020-01-11T17:05:07Z

  private void compact() {
    try {
      region.compact(true);
+      deleteCompactedHFiles();


We just delete archived files? Why even archive them then? Can we delay the delete at least?

Why we writing hfiles into WAL fs out of interest?

Apache9 · 2020-01-12T05:50:55Z

Add a TTL config for deleting the archived HFiles.

On why we put all things on the WAL filesystem, partly because that I want to put all the data under a single directory, which can hide the internal implementation details about procedure store, and easy to replace the old implementation with another implementation.

And another thing is about performance. The WAL filesystem is HDFS, and the HFile storage, if not the same one, is usually S3 or some other object storages, which are very slow, usually. And usually the size of the procedure store will be small, I think the cost is also fine.

Apache-HBase · 2020-01-12T16:37:51Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 4s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
+1 💚	mvninstall	5m 56s	master passed
+1 💚	compile	0m 59s	master passed
+1 💚	checkstyle	1m 31s	master passed
+1 💚	shadedjars	5m 1s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 38s	master passed
+0 🆗	spotbugs	4m 49s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	4m 47s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	5m 37s	the patch passed
+1 💚	compile	0m 57s	the patch passed
+1 💚	javac	0m 57s	the patch passed
+1 💚	checkstyle	1m 28s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedjars	4m 59s	patch has no errors when building our shaded downstream artifacts.
+1 💚	hadoopcheck	17m 6s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚	javadoc	0m 35s	the patch passed
+1 💚	findbugs	5m 2s	the patch passed
		_ Other Tests _
+1 💚	unit	154m 48s	hbase-server in the patch passed.
+1 💚	asflicense	0m 26s	The patch does not generate ASF License warnings.
		217m 44s

Subsystem	Report/Notes
Docker	Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/4/artifact/out/Dockerfile
GITHUB PR	#1022
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux d13a4b4c0e5b 4.15.0-66-generic #75-Ubuntu SMP Tue Oct 1 05:24:09 UTC 2019 x86_64 GNU/Linux
Build tool	maven
Personality	/home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1022/out/precommit/personality/provided.sh
git revision	master / `4ad12e0`
Default Java	1.8.0_181
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/4/testReport/
Max. process+thread count	4915 (vs. ulimit of 10000)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/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.

Apache9 · 2020-01-14T14:20:05Z

Ping @saintstack

busbey · 2020-01-14T17:38:17Z

On why we put all things on the WAL filesystem, partly because that I want to put all the data under a single directory, which can hide the internal implementation details about procedure store, and easy to replace the old implementation with another implementation.

And another thing is about performance. The WAL filesystem is HDFS, and the HFile storage, if not the same one, is usually S3 or some other object storages, which are very slow, usually. And usually the size of the procedure store will be small, I think the cost is also fine.

I like that this goes on the WAL filesystem and I think the justification is good. I went to check the docs to make sure the reasoning is spelled out as well there. Looks like the ref guide does not have details about the new procedure store and still has a bunch of stuff about the old one. Is there a JIRA I can watch to review when those docs are up?

Apache9 · 2020-01-15T14:04:43Z

On why we put all things on the WAL filesystem, partly because that I want to put all the data under a single directory, which can hide the internal implementation details about procedure store, and easy to replace the old implementation with another implementation.
And another thing is about performance. The WAL filesystem is HDFS, and the HFile storage, if not the same one, is usually S3 or some other object storages, which are very slow, usually. And usually the size of the procedure store will be small, I think the cost is also fine.

I like that this goes on the WAL filesystem and I think the justification is good. I went to check the docs to make sure the reasoning is spelled out as well there. Looks like the ref guide does not have details about the new procedure store and still has a bunch of stuff about the old one. Is there a JIRA I can watch to review when those docs are up?

I checked the ref guide but did not seen anything about the procedure store? It is just about the procedure v2 framework, and these thing are not changed...

busbey · 2020-01-15T15:42:00Z

I checked the ref guide but did not seen anything about the procedure store? It is just about the procedure v2 framework, and these thing are not changed...

http://hbase.apache.org/book.html#master.wal

saintstack · 2020-01-15T22:41:00Z

Filed HBASE-23697 for doc on RegionProcedureStore.

Apache9 · 2020-01-16T00:36:07Z

I checked the ref guide but did not seen anything about the procedure store? It is just about the procedure v2 framework, and these thing are not changed...

http://hbase.apache.org/book.html#master.wal

Oh, it is not under the section of proc-v2? ...

busbey · 2020-01-16T00:47:46Z

Correct; the docs are a mess.

Apache9 · 2020-01-16T08:53:44Z

Added a hbase.procedure.store.region.hfilecleaner.plugins, and add the default config to hbase-default.xml. The default value is org.apache.hadoop.hbase.master.cleaner.TimeToLiveHFileCleaner.

Apache-HBase · 2020-01-16T13:07:50Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 7s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 7 new or modified test files.
		_ master Compile Tests _
+0 🆗	mvndep	0m 32s	Maven dependency ordering for branch
+1 💚	mvninstall	5m 44s	master passed
+1 💚	compile	1m 39s	master passed
+1 💚	checkstyle	2m 9s	master passed
+0 🆗	refguide	6m 26s	branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚	shadedjars	5m 0s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	1m 11s	master passed
+0 🆗	spotbugs	5m 2s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	6m 22s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 14s	Maven dependency ordering for patch
+1 💚	mvninstall	5m 28s	the patch passed
+1 💚	compile	1m 38s	the patch passed
+1 💚	javac	1m 38s	the patch passed
-1 ❌	checkstyle	1m 30s	hbase-server: The patch generated 3 new + 103 unchanged - 0 fixed = 106 total (was 103)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	xml	0m 2s	The patch has no ill-formed XML file.
+0 🆗	refguide	6m 17s	patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚	shadedjars	5m 4s	patch has no errors when building our shaded downstream artifacts.
+1 💚	hadoopcheck	17m 14s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚	javadoc	1m 10s	the patch passed
+1 💚	findbugs	6m 40s	the patch passed
		_ Other Tests _
+1 💚	unit	3m 6s	hbase-common in the patch passed.
+1 💚	unit	3m 36s	hbase-procedure in the patch passed.
+1 💚	unit	155m 54s	hbase-server in the patch passed.
+1 💚	asflicense	1m 16s	The patch does not generate ASF License warnings.
		247m 17s

Subsystem	Report/Notes
Docker	Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/5/artifact/out/Dockerfile
GITHUB PR	#1022
Optional Tests	dupname asflicense javac javadoc unit refguide xml spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux b929b09c26eb 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-1022/out/precommit/personality/provided.sh
git revision	master / `19d3bed`
Default Java	1.8.0_181
refguide	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/5/artifact/out/branch-site/book.html
checkstyle	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/5/artifact/out/diff-checkstyle-hbase-server.txt
refguide	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/5/artifact/out/patch-site/book.html
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/5/testReport/
Max. process+thread count	5002 (vs. ulimit of 10000)
modules	C: hbase-common hbase-procedure hbase-server U: .
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/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.

Apache-HBase · 2020-01-16T19:46:43Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 2s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 1s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 7 new or modified test files.
		_ master Compile Tests _
+0 🆗	mvndep	0m 33s	Maven dependency ordering for branch
+1 💚	mvninstall	5m 41s	master passed
+1 💚	compile	1m 37s	master passed
+1 💚	checkstyle	2m 9s	master passed
+0 🆗	refguide	6m 26s	branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚	shadedjars	4m 59s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	1m 12s	master passed
+0 🆗	spotbugs	4m 49s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	6m 9s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 14s	Maven dependency ordering for patch
+1 💚	mvninstall	5m 28s	the patch passed
+1 💚	compile	1m 38s	the patch passed
+1 💚	javac	1m 38s	the patch passed
+1 💚	checkstyle	2m 7s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	xml	0m 1s	The patch has no ill-formed XML file.
+0 🆗	refguide	6m 19s	patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚	shadedjars	5m 2s	patch has no errors when building our shaded downstream artifacts.
+1 💚	hadoopcheck	17m 8s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚	javadoc	1m 11s	the patch passed
+1 💚	findbugs	6m 34s	the patch passed
		_ Other Tests _
+1 💚	unit	3m 8s	hbase-common in the patch passed.
+1 💚	unit	3m 35s	hbase-procedure in the patch passed.
+1 💚	unit	155m 19s	hbase-server in the patch passed.
+1 💚	asflicense	1m 9s	The patch does not generate ASF License warnings.
		246m 4s

Subsystem	Report/Notes
Docker	Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/6/artifact/out/Dockerfile
GITHUB PR	#1022
Optional Tests	dupname asflicense javac javadoc unit refguide xml spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux 608fe92c9c22 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-1022/out/precommit/personality/provided.sh
git revision	master / `edc5368`
Default Java	1.8.0_181
refguide	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/6/artifact/out/branch-site/book.html
refguide	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/6/artifact/out/patch-site/book.html
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/6/testReport/
Max. process+thread count	4602 (vs. ulimit of 10000)
modules	C: hbase-common hbase-procedure hbase-server U: .
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1022/6/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.

Apache9 · 2020-01-17T15:04:58Z

Ping @busbey @saintstack

saintstack

One nit.

saintstack · 2020-01-17T23:14:48Z

-    procedureStore =
-      new RegionProcedureStore(this, new MasterProcedureEnv.FsUtilsLeaseRecovery(this));
+    // Create cleaner thread pool
+    cleanerPool = new DirScanPool(conf);


Strange place to start cleanerPool down here in the procedure startup given it is used cleaning WALs and hfile...

But we have no choice as we need to use it in the next line...

I was thinking you'd call it up in the caller's method, in startServiceThreads, rather than down hidden in here in startProcedureExecutor... i.e. move the line up thee lines from where it was.

The startServiceThreads method is called after createProcedureExecutor, notice that, it is create, not start...

Apache9 self-assigned this Jan 11, 2020

Apache9 requested a review from saintstack January 11, 2020 07:15

Apache9 added the bug label Jan 11, 2020

This comment has been minimized.

Sign in to view

Apache9 force-pushed the HBASE-23680 branch from e8c29cb to 6d2b430 Compare January 11, 2020 12:41

This comment has been minimized.

Sign in to view

saintstack reviewed Jan 11, 2020

View reviewed changes

Apache9 force-pushed the HBASE-23680 branch from 6d2b430 to e9e831c Compare January 12, 2020 05:44

This comment has been minimized.

Sign in to view

Apache9 force-pushed the HBASE-23680 branch from e9e831c to dbe7d1d Compare January 12, 2020 12:52

busbey requested changes Jan 14, 2020

View reviewed changes

Comment thread ...src/main/java/org/apache/hadoop/hbase/procedure2/store/region/RegionFlusherAndCompactor.java Outdated

Apache9 force-pushed the HBASE-23680 branch from dbe7d1d to 936c952 Compare January 16, 2020 08:51

HBASE-23680 RegionProcedureStore missing cleaning of hfile archive

ee1d497

Apache9 force-pushed the HBASE-23680 branch from 936c952 to ee1d497 Compare January 16, 2020 15:31

saintstack approved these changes Jan 17, 2020

View reviewed changes

Apache9 merged commit 167892c into apache:master Jan 18, 2020

Uh oh!

Conversation

Apache9 commented Jan 11, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

saintstack Jan 11, 2020

Choose a reason for hiding this comment

Uh oh!

saintstack Jan 11, 2020

Choose a reason for hiding this comment

Uh oh!

Apache9 commented Jan 12, 2020

Uh oh!

This comment has been minimized.

Apache-HBase commented Jan 12, 2020

Uh oh!

Apache9 commented Jan 14, 2020

Uh oh!

Uh oh!

busbey commented Jan 14, 2020

Uh oh!

Apache9 commented Jan 15, 2020

Uh oh!

busbey commented Jan 15, 2020

Uh oh!

saintstack commented Jan 15, 2020

Uh oh!

Apache9 commented Jan 16, 2020

Uh oh!

busbey commented Jan 16, 2020

Uh oh!

Apache9 commented Jan 16, 2020

Uh oh!

Apache-HBase commented Jan 16, 2020

Uh oh!

Apache-HBase commented Jan 16, 2020

Uh oh!

Apache9 commented Jan 17, 2020

Uh oh!

saintstack left a comment

Choose a reason for hiding this comment

Uh oh!

saintstack Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

Apache9 Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

saintstack Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

Apache9 Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants