HBASE-24998 Introduce a ReplicationSourceController interface … #2364
Conversation
This file contains 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. |
saintstack
reviewed
Sep 8, 2020
| @@ -979,6 +979,8 @@ | |||
| /* | |||
| * cluster replication constants. | |||
| */ | |||
| public static final String REPLICATION_OFFLOAD_ENABLE_KEY = "hbase.replication.offload.enabled"; | |||
| public static final boolean REPLICATION_OFFLOAD_ENABLE_DEFAULT = false; | |||
There was a problem hiding this comment.
Only used in ReplicationSourceManager so define these there?
There was a problem hiding this comment.
In the next patches, this may be used in other place, too.
| @@ -0,0 +1,31 @@ | |||
| package org.apache.hadoop.hbase.replication; | |||
| * Used by {@link ReplicationSource} or {@link RecoveredReplicationSource}. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public interface ReplicationSourceOverallController { |
There was a problem hiding this comment.
Can it just be ReplicationSoruceController? Drop the 'Overall'?
| * Returns the maximum size in bytes of edits held in memory which are pending replication | ||
| * across all sources inside this RegionServer or ReplicationServer. | ||
| */ | ||
| long getTotalBufferLimit(); |
There was a problem hiding this comment.
Is this a 'size' rather than a 'limit'? It is count of how many bytes are currently accumulated in replication source memory?
There was a problem hiding this comment.
Maybe the comment is just in the wrong place.. should be on the next data member?
There was a problem hiding this comment.
This was copied from the ReplicationSourceManager. And it is the total limit, not the currently size.
|
|
||
| AtomicLong getTotalBufferUsed(); | ||
|
|
||
| MetricsReplicationGlobalSourceSource getGlobalMetrics(); |
There was a problem hiding this comment.
What sort of 'global' metrics? This is metrics for all replication sources?
| this.totalBufferLimit = conf.getLong(HConstants.REPLICATION_SOURCE_TOTAL_BUFFER_KEY, | ||
| HConstants.REPLICATION_SOURCE_TOTAL_BUFFER_DFAULT); | ||
| this.globalMetrics = globalMetrics; | ||
| this.replicationOffload = conf.getBoolean(HConstants.REPLICATION_OFFLOAD_ENABLE_KEY, |
There was a problem hiding this comment.
I should go back to the design but we can't just have the offload be a ReplicationSource implementation? Because you need to span all ReplicationSources?
There was a problem hiding this comment.
In the design, the RegionServer not need to start all ReplicationSources. It only need to store its WAL path to the ReplicationQueueStorage. And the ReplicationServer will fetch the WAL paths and start all ReplicationSources.
| @@ -244,7 +286,7 @@ private void adoptAbandonedQueues() { | |||
| * </ol> | |||
| * @param peerId the id of replication peer | |||
| */ | |||
| public void addPeer(String peerId) throws IOException { | |||
| void addPeer(String peerId) throws IOException { | |||
There was a problem hiding this comment.
Funny, I want it to stay public in my current work (meta region replicas). Can move it back in my patch.
There was a problem hiding this comment.
I want to call it on Region open. Let me see if I can move where I make the call.
| @@ -268,7 +310,7 @@ public void addPeer(String peerId) throws IOException { | |||
| * </ol> | |||
| * @param peerId the id of the replication peer | |||
| */ | |||
| public void removePeer(String peerId) { | |||
| void removePeer(String peerId) { | |||
...src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
Show resolved
Hide resolved
| @@ -267,10 +267,11 @@ public Path getCurrentPath() { | |||
| //returns false if we've already exceeded the global quota | |||
| private boolean checkQuota() { | |||
| // try not to go over total quota | |||
| if (source.manager.getTotalBufferUsed().get() > source.manager.getTotalBufferLimit()) { | |||
| if (source.overallController.getTotalBufferUsed().get() > source.overallController | |||
There was a problem hiding this comment.
Yeah, if the Controller is in the ReplicationSourceManager, then the 'overall' will be redundant?
infraio
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
infraio
force-pushed
the
HBASE-24998
branch
2 times, most recently
from
99503d1
to
ac62e96
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. |
…and decouple ReplicationSourceManager and ReplicationSource
infraio
force-pushed
the
HBASE-24998
branch
from
ac62e96
to
069ed71
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Ping @saintstack . Any other concerns? |
mymeiyi
approved these changes
Sep 19, 2020
asfgit
pushed a commit
that referenced
this pull request
Sep 23, 2020
…and decouple ReplicationSourceManager and ReplicationSource (#2364) Signed-off-by: meiyi <myimeiyi@gmail.com>
asfgit
pushed a commit
that referenced
this pull request
Sep 24, 2020
…and decouple ReplicationSourceManager and ReplicationSource (#2364) Signed-off-by: meiyi <myimeiyi@gmail.com>
infraio
added a commit
that referenced
this pull request
Sep 26, 2020
…and decouple ReplicationSourceManager and ReplicationSource (#2364) Signed-off-by: meiyi <myimeiyi@gmail.com>
asfgit
pushed a commit
that referenced
this pull request
Sep 29, 2020
…and decouple ReplicationSourceManager and ReplicationSource (#2364) Signed-off-by: meiyi <myimeiyi@gmail.com>
asfgit
pushed a commit
that referenced
this pull request
Oct 22, 2020
…and decouple ReplicationSourceManager and ReplicationSource (#2364) Signed-off-by: meiyi <myimeiyi@gmail.com>
infraio
added a commit
that referenced
this pull request
Nov 9, 2020
…and decouple ReplicationSourceManager and ReplicationSource (#2364) Signed-off-by: meiyi <myimeiyi@gmail.com>
ddupg
pushed a commit
that referenced
this pull request
Feb 23, 2021
…and decouple ReplicationSourceManager and ReplicationSource (#2364) Signed-off-by: meiyi <myimeiyi@gmail.com>
ddupg
pushed a commit
that referenced
this pull request
Jul 9, 2021
…and decouple ReplicationSourceManager and ReplicationSource (#2364) Signed-off-by: meiyi <myimeiyi@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…and decouple ReplicationSourceManager and ReplicationSource