HBASE-22380 break circle replication when doing bulkload by wchevreuil · Pull Request #566 · apache/hbase

wchevreuil · 2019-08-30T15:00:35Z

PR for master branch

Apache-HBase · 2019-09-02T18:00:57Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	40	Docker mode activated.
		_ Prechecks _
+1	dupname	1	No case conflicting files found.
+1	hbaseanti	0	Patch does not have any anti-patterns.
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 2 new or modified test files.
		_ master Compile Tests _
0	mvndep	34	Maven dependency ordering for branch
+1	mvninstall	332	master passed
+1	compile	124	master passed
+1	checkstyle	154	master passed
+1	shadedjars	314	branch has no errors when building our shaded downstream artifacts.
+1	javadoc	81	master passed
0	spotbugs	271	Used deprecated FindBugs config; considering switching to SpotBugs.
+1	findbugs	520	master passed
		_ Patch Compile Tests _
0	mvndep	19	Maven dependency ordering for patch
+1	mvninstall	382	the patch passed
+1	compile	118	the patch passed
+1	cc	118	the patch passed
+1	javac	118	the patch passed
-1	checkstyle	82	hbase-server: The patch generated 1 new + 283 unchanged - 0 fixed = 284 total (was 283)
+1	whitespace	1	The patch has no whitespace issues.
+1	shadedjars	289	patch has no errors when building our shaded downstream artifacts.
+1	hadoopcheck	1000	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1	hbaseprotoc	242	the patch passed
-1	javadoc	103	hbase-server generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
-1	findbugs	537	hbase-server generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
		_ Other Tests _
+1	unit	65	hbase-protocol-shaded in the patch passed.
+1	unit	181	hbase-client in the patch passed.
-1	unit	10539	hbase-server in the patch failed.
+1	asflicense	86	The patch does not generate ASF License warnings.
		16575

Reason	Tests
FindBugs	module:hbase-server
	org.apache.hadoop.hbase.replication.regionserver.ReplicationSink.replicateEntries(List, CellScanner, String, String, String) makes inefficient use of keySet iterator instead of entrySet iterator At ReplicationSink.java:makes inefficient use of keySet iterator instead of entrySet iterator At ReplicationSink.java:[line 258]
Failed junit tests	hadoop.hbase.tool.TestBulkLoadHFilesSplitRecovery
	hadoop.hbase.tool.TestSecureBulkLoadHFilesSplitRecovery
	hadoop.hbase.regionserver.TestHRegionWithInMemoryFlush

Subsystem	Report/Notes
Docker	Client=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/7/artifact/out/Dockerfile
GITHUB PR	#566
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc
uname	Linux feb8ff49f0f1 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/HBase-PreCommit-GitHub-PR_PR-566/out/precommit/personality/provided.sh
git revision	master / `8e8bd8b`
Default Java	1.8.0_181
checkstyle	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/7/artifact/out/diff-checkstyle-hbase-server.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/7/artifact/out/diff-javadoc-javadoc-hbase-server.txt
findbugs	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/7/artifact/out/new-findbugs-hbase-server.html
unit	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/7/artifact/out/patch-unit-hbase-server.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/7/testReport/
Max. process+thread count	4500 (vs. ulimit of 10000)
modules	C: hbase-protocol-shaded hbase-client hbase-server U: .
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/7/console
versions	git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by	Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

busbey · 2019-09-03T14:53:45Z

how is the implementation here different than on #494?

wchevreuil · 2019-09-04T14:09:23Z

Concept is the same, but there were major changes between branch-2 and master code bases that required some re-work to come with a PR to master. For example, there's no LoadIncrementalHFiles, the most equivalent class is BulkLoadHFilesTool. Same for SecureBulkLoadClient, which was removed on master branch, being replaced by AsyncClusterConnection/AsyncClusterConnectionImpl methods.

joshelser

Generally looks OK to me. A couple of comments/questions/requests, but none of them serious.

I think the only thing that worries me is that the test doesn't seem to be doing a replication "ring" of 3 clusters. I thought that was the intent reading HBASE-22380, but maybe I missed something over there.

joshelser · 2019-09-10T20:53:15Z

+ //We have 3 bulk load events (1 initiated on each cluster).
+ //Each event gets 3 counts (the originator cluster, plus the two peers),
+ //so BULK_LOADS_COUNT expected value is 3 * 3 = 9.
+ assertEquals(9, BULK_LOADS_COUNT.get());


I'm thinking about debugging this if/when it breaks. Is there any way that we could expand your Observer to know which Clusters got their bulk loaded files and how many? e.g. a Map of ClusterID to counts of files bulkloaded (or even a list of the file names that were bulkloaded)

Do you mean to add extra validation per file bullkloaded on this test? Or are you talking about a general bulk load observer to be implemented on a separate jira?

Not so much validation, but just proactive-debugging.
For example, if the test fails was it because the hfiles never made it from A to B or B to C?
I think some counters in a bulk loader observer would be a nice way to just proactively log "I saw 3 bulk loads for clusterA, 3 for clusterB, and 0 for cluster" which would make debugging much easier. If it's not that straightforward to capture/log, don't worry about it :)

That's a good idea actually. Pushed a change that adds a counter to this CP and prints a debug message on each completed bulkload, for each cluster.

joshelser · 2019-09-10T20:53:45Z

+ value = Bytes.toBytes("v3");
+ assertBulkLoadConditions(row, value, UTIL3, peer1TestTable, peer2TestTable, peer3TestTable);
+ //Additional wait to make sure no extra bulk load happens
+ Thread.sleep(400);


Any way we could know when HBase is done replicating? Could we figure this out from the replication queue?

Problem is that we want to make sure no "extra" replication will occur. Checking replication queue would require extra code to read/monitor it from ZK, I guess, and we still would need to do some wait, then certify no new items were enqueued.

Just to confirm, we don't have any Observer hook for when a bulkload-via-replication request comes in? so we can't just have a latch that ensure each server sees the correct number of events?

Just to confirm, we don't have any Observer hook for when a bulkload-via-replication request comes in?
No, just general bulkload events, via preBulkLoadHFile/postBulkLoadHFile.
so we can't just have a latch that ensure each server sees the correct number of events?
We are already using a latch for the expected amount of bulk load events. This wait here is for the case where we have infinite loops. Simply relying on countdown latch would not suffice, we would pass the test even if we are in a loop situation.

Sure you could. Just make a latch that once we pass the expected number it starts throwing exceptions or calls Assert.fail

We would still need to wait a bit over once we reached the expected number, right? Otherwise, we may pass the test before an additional, unwanted bulkload event happens to be accounted in the latch.

ah. yeah unless we can check the replication status to show that we're up to date.

Ick, yeah, that's hard to capture. I like the idea of proactively failing when we get more bulk loads than we expect (we can probably rule out retriable bulk load errors in a contrived unit test like this). Don't consider this to be some blocking request for change -- just an open question :)

(we can probably rule out retriable bulk load errors in a contrived unit test like this).
Moved the BULK_LOADS_COUNT increment to postBulkLoadHFile Observer method. That way, we would only be counting succeeded bulkloads, ignoring failed ones, so if it's a retry, the failed attempt didn't get count.
But the problem of how to check something that shouldn't happen anytime in the future without the wait/sleep seems unsolvable to me.

hehe, yeah. The only thing I could think of would be instrumenting the code that "skips" replicating to a cluster which already got it, but that's pretty down in the weeds, I'd say.
I think you're good :)

Apache-HBase · 2019-09-12T16:16:39Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
💙	reexec	0m 0s	Docker mode activated.
💔	patch	0m 6s	#566 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/in-progress/precommit-patchnames for help.

Subsystem	Report/Notes
GITHUB PR	#566
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/8/console
versions	git=2.17.1
Powered by	Apache Yetus 0.11.0 https://yetus.apache.org

This message was automatically generated.

wchevreuil · 2019-09-16T16:11:30Z

Ok, so would it be ok as it is now?

…or branch-2

Apache-HBase · 2019-09-17T18:30:47Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
💙	reexec	0m 0s	Docker mode activated.
💔	patch	0m 5s	#566 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/in-progress/precommit-patchnames for help.

Subsystem	Report/Notes
GITHUB PR	#566
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/9/console
versions	git=2.17.1
Powered by	Apache Yetus 0.11.0 https://yetus.apache.org

This message was automatically generated.

…kloads happened for each cluster, in order to help with debugging on eventual test failures

Apache-HBase · 2019-09-20T16:54:22Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
💙	reexec	0m 0s	Docker mode activated.
💔	patch	0m 5s	#566 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/in-progress/precommit-patchnames for help.

Subsystem	Report/Notes
GITHUB PR	#566
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-566/10/console
versions	git=2.17.1
Powered by	Apache Yetus 0.11.0 https://yetus.apache.org

This message was automatically generated.

HBASE-22380 break circle replication when doing bulkload
d2a3250

wchevreuil requested review from Apache9, anoopsjohn, apurtell, petersomogyi and saintstack August 30, 2019 15:00

This comment has been minimized.
Sign in to view

joshelser reviewed Sep 10, 2019
View reviewed changes

busbey requested changes Sep 11, 2019
View reviewed changes

Comment threadhbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestBulkLoadReplication.java Outdated
Comment threadhbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java

addressing some of latest comments by jelser and busbey.
90a713f

addressing findbugs warning per Busbey's suggestion on the other PR f…
cce518a
…or branch-2

adding counter for co-processor, so that we can identify how many bul…
b3cd273
…kloads happened for each cluster, in order to help with debugging on eventual test failures

joshelser approved these changes Sep 20, 2019
View reviewed changes

wchevreuil merged commit 3ae353c into apache:masterSep 21, 2019

Uh oh!

Conversation

wchevreuil commented Aug 30, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Apache-HBase commented Sep 2, 2019

Uh oh!

busbey commented Sep 3, 2019

Uh oh!

wchevreuil commented Sep 4, 2019

Uh oh!

joshelser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchevreuilSep 12, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Apache-HBase commented Sep 12, 2019

Uh oh!

wchevreuil commented Sep 16, 2019

Uh oh!

Apache-HBase commented Sep 17, 2019

Uh oh!

Apache-HBase commented Sep 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wchevreuilSep 12, 2019•
edited
Loading