HBASE-23683 Make HBaseInterClusterReplicationEndpoint more extensible… #1047

wchevreuil · 2020-01-15T13:05:24Z

Had opened a separate PR for branch-2, as it doesn't have AsyncClusterConnection, requiring changes on basically the whole connection creation code. One thought I had just after I had already merged previous PR in master is that the two additional create methods should refer to Connection, instead of ClusterConnection or AsyncClusterConnection, as that would shield HBaseInterClusterReplicationEndpoint extensions from changes between 2.x and 3.x.

Ping @bharathv

…apache#1027) Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: Josh Elser <[email protected]> Signed-off-by: binlijin <[email protected]>

Apache-HBase · 2020-01-15T16:34:57Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 37s	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.
-0 ⚠️	test4tests	0m 0s	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ branch-2 Compile Tests _
+1 💚	mvninstall	5m 21s	branch-2 passed
+1 💚	compile	0m 57s	branch-2 passed
+1 💚	checkstyle	1m 19s	branch-2 passed
+1 💚	shadedjars	4m 13s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 37s	branch-2 passed
+0 🆗	spotbugs	3m 31s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	3m 30s	branch-2 passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 44s	the patch passed
+1 💚	compile	0m 55s	the patch passed
+1 💚	javac	0m 55s	the patch passed
+1 💚	checkstyle	1m 17s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedjars	4m 13s	patch has no errors when building our shaded downstream artifacts.
+1 💚	hadoopcheck	15m 21s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚	javadoc	0m 36s	the patch passed
+1 💚	findbugs	3m 38s	the patch passed
		_ Other Tests _
-1 ❌	unit	150m 3s	hbase-server in the patch failed.
+1 💚	asflicense	0m 35s	The patch does not generate ASF License warnings.
		204m 24s

Reason	Tests
Failed junit tests	hadoop.hbase.client.TestAsyncTableGetMultiThreaded

Subsystem	Report/Notes
Docker	Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/1/artifact/out/Dockerfile
GITHUB PR	#1047
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux 73d99e37e9b9 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/Base-PreCommit-GitHub-PR_PR-1047/out/precommit/personality/provided.sh
git revision	branch-2 / `ae95b1f`
Default Java	1.8.0_181
unit	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/1/artifact/out/patch-unit-hbase-server.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/1/testReport/
Max. process+thread count	5893 (vs. ulimit of 10000)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/1/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.

bharathv

Got a question..

bharathv · 2020-01-15T16:59:35Z

...a/org/apache/hadoop/hbase/replication/regionserver/HBaseInterClusterReplicationEndpoint.java

+   * so defining createReplicationSinkManager as protected for possible overridings.
+   */
+  protected ReplicationSinkManager createReplicationSinkManager(Connection conn) {
+    return new ReplicationSinkManager((ClusterConnection) conn, this.ctx.getPeerId(),


nit: redundant cast to conn.

Compilation fails without this cast, as ReplicationSinkManager expects ClusterConnection.

sorry, I meant pass ClusterConnection directly, but I guess that brings us back to your other comment as to why you want to use generic "Connection" object.

bharathv · 2020-01-15T17:01:15Z

...a/org/apache/hadoop/hbase/replication/regionserver/HBaseInterClusterReplicationEndpoint.java

    // replication and make replication specific settings such as compression or codec to use
    // passing Cells.
-    this.conn = (ClusterConnection) ConnectionFactory.createConnection(this.conf);
+    this.conn = (ClusterConnection) createConnection(this.conf);


Do we need force createConnection() to return a connection of type ClusterConnection? Otherwise, theoretically overrides can return any type of Connection (may or may not be ClusterConnection) and the type cast here fails. Just a thought.

Yeah, that kind of restrict the extensibility, which is what we are trying to improve here, but addressing that would require an extra refactoring effort, just as @joshelser mentioned on the master PR. For example, in master branch, we are forced to return AsyncClusterConnection.

that kind of restrict the extensibility, which is what we are trying to improve here,

Hmm.. May be I'm missing something, what does your use case look like here where you have an override that does a generic Connection rather than (some type of) ClusterConnection? I'm curious how it restricts extensibility because to use this, it has to be a ClusterConnection, otherwise it fails at runtime, no?

what does your use case look like here where you have an override that does a generic Connection rather than (some type of) ClusterConnection?

I have a HBaseInterClusterReplicationEndpoint extension where I'm still using ConnectionFactory for creating connections, but I use different factory method than what is used by HBaseInterClusterReplicationEndpoint. I don't really care about which connection implementation the factory is returning, I just rely on whatever is the default (in 2.x, it's ClusterConnection). I noticed, though, that default connection type changed between 2.x and 3.0, from ClusterConnection to AsycnClusterConnection. So, if we keep create methods referencing generic Connection only, I won't need to change my HBaseInterClusterReplicationEndpoint extension if I upgrade to hbase 3.x (provided, of course, that HBaseInterClusterReplicationEndpoint.conn variable also changes to same default returned by ConnectionFactory).

Oh I see.. I think I get the problem. Basically you are just relying on ConnectionFactory and not worried about the type it is returning. Ya, refactoring it will be a bigger patch.

nit: How about adding a preconditions check.. Preconditions.checkState(returnedConn instanceof ClusterConnection) and back it up with a comment why it could fail?

bharathv · 2020-01-15T17:01:50Z

...a/org/apache/hadoop/hbase/replication/regionserver/HBaseInterClusterReplicationEndpoint.java

  private boolean isSerial = false;

+  /*
+   * Some implementations of HBaseInterClusterReplicationEndpoint may require instantiate different


nit: instantiating..

bharathv

I'm +1 once my nits are addressed. Thanks for patiently answering my questions.

bharathv · 2020-01-16T17:26:55Z

...a/org/apache/hadoop/hbase/replication/regionserver/HBaseInterClusterReplicationEndpoint.java

    // replication and make replication specific settings such as compression or codec to use
    // passing Cells.
-    this.conn = (ClusterConnection) ConnectionFactory.createConnection(this.conf);
+    this.conn = (ClusterConnection) createConnection(this.conf);


Oh I see.. I think I get the problem. Basically you are just relying on ConnectionFactory and not worried about the type it is returning. Ya, refactoring it will be a bigger patch.

nit: How about adding a preconditions check.. Preconditions.checkState(returnedConn instanceof ClusterConnection) and back it up with a comment why it could fail?

wchevreuil · 2020-01-20T14:11:08Z

Thanks for the suggestions, @bharathv , had addressed it on last commit. @joshelser @binlijin , would you guys think this is good to go on branch-2?

I think we would also need an addendum commit for master branch with the generic Connection reference and @bharathv last suggestions.

Apache-HBase · 2020-01-20T17:31:48Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 36s	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.
-0 ⚠️	test4tests	0m 0s	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ branch-2 Compile Tests _
+1 💚	mvninstall	5m 22s	branch-2 passed
+1 💚	compile	0m 55s	branch-2 passed
+1 💚	checkstyle	1m 11s	branch-2 passed
+1 💚	shadedjars	4m 15s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 39s	branch-2 passed
+0 🆗	spotbugs	3m 37s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	3m 34s	branch-2 passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 49s	the patch passed
+1 💚	compile	0m 56s	the patch passed
+1 💚	javac	0m 56s	the patch passed
-1 ❌	checkstyle	1m 7s	hbase-server: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedjars	4m 15s	patch has no errors when building our shaded downstream artifacts.
+1 💚	hadoopcheck	15m 13s	Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚	javadoc	0m 34s	the patch passed
+1 💚	findbugs	3m 35s	the patch passed
		_ Other Tests _
+1 💚	unit	147m 5s	hbase-server in the patch passed.
+1 💚	asflicense	0m 35s	The patch does not generate ASF License warnings.
		201m 1s

Subsystem	Report/Notes
Docker	Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/2/artifact/out/Dockerfile
GITHUB PR	#1047
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux 632793a7bba2 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/Base-PreCommit-GitHub-PR_PR-1047/out/precommit/personality/provided.sh
git revision	branch-2 / `8f292a8`
Default Java	1.8.0_181
checkstyle	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/2/artifact/out/diff-checkstyle-hbase-server.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/2/testReport/
Max. process+thread count	4602 (vs. ulimit of 10000)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/2/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.

wchevreuil · 2020-01-20T18:20:30Z

The checkstyle import order complaint doesn't look correct.

bharathv · 2020-01-20T18:32:24Z

The checkstyle import order complaint doesn't look correct.

Try moving "import org.apache.hbase.thirdparty.com.google.common.base.Preconditions" towards the end..It is likely because we enforce grouping for this third party package.

bharathv

Thanks for addressing my comments.

binlijin

LGTM +1

wchevreuil · 2020-01-21T14:47:52Z

Try moving "import org.apache.hbase.thirdparty.com.google.common.base.Preconditions" towards the end..It is likely because we enforce grouping for this third party package.

Let's give it a try.

Apache-HBase · 2020-01-21T18:13:36Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 35s	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.
-0 ⚠️	test4tests	0m 0s	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ branch-2 Compile Tests _
+1 💚	mvninstall	5m 23s	branch-2 passed
+1 💚	compile	0m 54s	branch-2 passed
+1 💚	checkstyle	1m 9s	branch-2 passed
+1 💚	shadedjars	4m 13s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 36s	branch-2 passed
+0 🆗	spotbugs	3m 37s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	3m 34s	branch-2 passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 51s	the patch passed
+1 💚	compile	0m 56s	the patch passed
+1 💚	javac	0m 56s	the patch passed
+1 💚	checkstyle	1m 8s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	shadedjars	4m 15s	patch has no errors when building our shaded downstream artifacts.
+1 💚	hadoopcheck	15m 20s	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	3m 36s	the patch passed
		_ Other Tests _
+1 💚	unit	149m 7s	hbase-server in the patch passed.
+1 💚	asflicense	0m 33s	The patch does not generate ASF License warnings.
		202m 57s

Subsystem	Report/Notes
Docker	Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/3/artifact/out/Dockerfile
GITHUB PR	#1047
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname	Linux 1a743339b689 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/Base-PreCommit-GitHub-PR_PR-1047/out/precommit/personality/provided.sh
git revision	branch-2 / `c165699`
Default Java	1.8.0_181
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/3/testReport/
Max. process+thread count	4673 (vs. ulimit of 10000)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1047/3/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.

#1047) Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: binlijin <[email protected]> (cherry picked from commit 62e3409)

apache#1047) Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: binlijin <[email protected]>

apache#1047) Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: binlijin <[email protected]> (cherry picked from commit 62e3409) (cherry picked from commit 877564c) Change-Id: I3b1d5c76861465059aa3df2b679c91f6e5f20bb1

HBASE-23683 Make HBaseInterClusterReplicationEndpoint more extensible (…

f9c7734

…apache#1027) Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: Josh Elser <[email protected]> Signed-off-by: binlijin <[email protected]>

wchevreuil requested review from binlijin and joshelser January 15, 2020 13:05

bharathv reviewed Jan 15, 2020

View reviewed changes

bharathv reviewed Jan 16, 2020

View reviewed changes

Addressing last round of reviews

0868dfd

bharathv approved these changes Jan 20, 2020

View reviewed changes

binlijin approved these changes Jan 21, 2020

View reviewed changes

changed import order

fea133b

wchevreuil merged commit 62e3409 into apache:branch-2 Jan 22, 2020

infraio pushed a commit to infraio/hbase that referenced this pull request Aug 17, 2020

HBASE-23683 Make HBaseInterClusterReplicationEndpoint more extensible… (

d0ac93a

apache#1047) Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: binlijin <[email protected]>

HBASE-23683 Make HBaseInterClusterReplicationEndpoint more extensible… #1047

HBASE-23683 Make HBaseInterClusterReplicationEndpoint more extensible… #1047

Uh oh!

Conversation

wchevreuil commented Jan 15, 2020

Uh oh!

Apache-HBase commented Jan 15, 2020

Uh oh!

bharathv left a comment

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!

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!

bharathv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchevreuil commented Jan 20, 2020

Uh oh!

Apache-HBase commented Jan 20, 2020

Uh oh!

wchevreuil commented Jan 20, 2020

Uh oh!

bharathv commented Jan 20, 2020

Uh oh!

bharathv left a comment

Choose a reason for hiding this comment

Uh oh!

binlijin left a comment

Choose a reason for hiding this comment

Uh oh!

wchevreuil commented Jan 21, 2020

Uh oh!

Apache-HBase commented Jan 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants