HBASE-24683 Add a basic ReplicationServer which only implement Replic… #2111
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
infraio
reviewed
Aug 10, 2020
| */ | ||
| @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.TOOLS) | ||
| @SuppressWarnings({ "deprecation"}) | ||
| public class HReplicationServer extends Thread implements Server, RegionServerServices { |
Contributor
There was a problem hiding this comment.
Do not need to implements RegionServerServices?
Contributor
There was a problem hiding this comment.
Have similar feeling, given the high number of methods where an implementation is not applicable in this context.
Member
Author
There was a problem hiding this comment.
Yean, let me remove implements RegionServerServices.
infraio
reviewed
Aug 10, 2020
| */ | ||
| @InterfaceAudience.Private | ||
| @SuppressWarnings("deprecation") | ||
| public class ReplicationServerRpcServices implements HBaseRPCErrorHandler, |
Contributor
There was a problem hiding this comment.
No need to impl AdminService.BlockingInterface?
Contributor
|
@ddupg Please rebase HBASE-24666 and push again. |
wchevreuil
reviewed
Aug 10, 2020
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Sorry, this a large PR, may rather do incremental reviews. Had put some minor observations, but one thing I had not understood so far is how ReplicationServers will choose which specific WALs each running server would track. Can you shed some light on this? Perhaps, pointing me to some code snippets where this is actually implemented.
|
|
||
| protected final Configuration conf; | ||
|
|
||
| private ReplicationSinkService replicationSinkHandler; |
Contributor
There was a problem hiding this comment.
At the level of HReplicationServer, we only need to call methods from ReplicationService. I found a bit confusing that we were referring directly a sink only here, until I realised replicationSinkHandler is an instance of Replication. Can we just refer to ReplicationService interface here?
hbase-server/src/main/java/org/apache/hadoop/hbase/replication/HReplicationServer.java
Outdated
Show resolved
Hide resolved
Comment on lines
608
to
336
| private void setupReplication() throws IOException { | ||
| // Instantiate replication if replication enabled. Pass it the log directories. | ||
| createNewReplicationInstance(conf, this); | ||
| } | ||
|
|
||
| /** | ||
| * Load the replication executorService objects, if any | ||
| */ | ||
| private static void createNewReplicationInstance(Configuration conf, HReplicationServer server) | ||
| throws IOException { | ||
| // read in the name of the sink replication class from the config file. | ||
| String sinkClassname = conf.get(HConstants.REPLICATION_SINK_SERVICE_CLASSNAME, | ||
| HConstants.REPLICATION_SERVICE_CLASSNAME_DEFAULT); | ||
|
|
||
| server.replicationSinkHandler = newReplicationInstance(sinkClassname, | ||
| ReplicationSinkService.class, conf, server); | ||
| } |
Contributor
There was a problem hiding this comment.
Why having these two methods? setupReplication apparently doing nothing extra to _ createNewReplicationInstance_.
Member
Author
There was a problem hiding this comment.
Oh, let me remove one of them.
| throw new IOException("Could not find class for " + classname); | ||
| } | ||
| T service = ReflectionUtils.newInstance(clazz, conf); | ||
| service.initialize(server, null, null, null, null); |
Contributor
There was a problem hiding this comment.
Why all those null params? Can we remove those from the interface?
Member
Author
There was a problem hiding this comment.
Those null params are about replication source, not replication sink. For this issue, ReplicationServer is only responsible for sink.
Those extra params are useless for sink, and I also think it’s weird that ReplicationSinkService use the same params with ReplicationSourceService. But if we need to refactor these interfaces, that should be in an another issue.
| } | ||
|
|
||
| @Override | ||
| public FlushRequester getFlushRequester() { |
Contributor
There was a problem hiding this comment.
Should throw UnsupportedOperationException? (And same applies to all non implemented methods currently returning null).
| */ | ||
| @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.TOOLS) | ||
| @SuppressWarnings({ "deprecation"}) | ||
| public class HReplicationServer extends Thread implements Server, RegionServerServices { |
Contributor
There was a problem hiding this comment.
Have similar feeling, given the high number of methods where an implementation is not applicable in this context.
| final RpcServerInterface rpcServer; | ||
| final InetSocketAddress isa; | ||
|
|
||
| @VisibleForTesting |
Contributor
There was a problem hiding this comment.
There's been a discussion, lately, about removing VisibleForTesting annotation, since it's not mentioned on our compatibility promises. The outcome is that we should rather rely on IA.Private only, and avoid VisibleForTesting.
Member
Author
For this issue, just add a basic ReplicationServer, which is only responsible for replication sink, not for source yet. So for now, it only holds and implements sink stuff. In future issues, will move replication source stuff here. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ddupg
force-pushed
the
HBASE-24683
branch
2 times, most recently
from
e4b4ab3 to
a9d89a7
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…ationSink Service
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
infraio
pushed a commit
to infraio/hbase
that referenced
this pull request
Sep 8, 2020
…ationSink Service (apache#2111) Signed-off-by: Guanghao Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 14, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 23, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 24, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <[email protected]>
infraio
pushed a commit
that referenced
this pull request
Sep 26, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 29, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Oct 22, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <[email protected]>
infraio
pushed a commit
that referenced
this pull request
Nov 9, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <[email protected]>
ddupg
added a commit
that referenced
this pull request
Feb 23, 2021
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <[email protected]>
ddupg
added a commit
that referenced
this pull request
Jul 9, 2021
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ationSink Service