Uh oh!
There was an error while loading. Please reload this page.
HBASE-24683 Add a basic ReplicationServer which only implement Replic… - #2111
Merged
Conversation
Apache-HBase
commented
Jul 21, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 21, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 21, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 22, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 22, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 22, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 10, 2020
💔 -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.
MemberAuthor
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?
infraio
commented
Aug 10, 2020
Contributor
@ddupg Please rebase HBASE-24666 and push again. |
wchevreuil
left a comment
Contributor
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?
Uh oh!
There was an error while loading. Please reload this page.
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_.
| 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?
MemberAuthor
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.
ddupg
commented
Aug 21, 2020
MemberAuthor
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. |
Apache-HBase
commented
Aug 21, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 21, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 21, 2020
🎊 +1 overall
This message was automatically generated. |
ddupgforce-pushed
the
HBASE-24683
branch
2 times, most recently
from
September 4, 2020 03:39
e4b4ab3 to
a9d89a7CompareApache-HBase
commented
Sep 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 4, 2020
🎊 +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 <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 14, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 23, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 24, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <zghao@apache.org>
infraio pushed a commit
that referenced
this pull request
Sep 26, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Sep 29, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <zghao@apache.org>
asfgit pushed a commit
that referenced
this pull request
Oct 22, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <zghao@apache.org>
infraio pushed a commit
that referenced
this pull request
Nov 9, 2020
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <zghao@apache.org>
ddupg added a commit
that referenced
this pull request
Feb 23, 2021
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <zghao@apache.org>
ddupg added a commit
that referenced
this pull request
Jul 9, 2021
…ationSink Service (#2111) Signed-off-by: Guanghao Zhang <zghao@apache.org>
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
Sign up for freeto 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