HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers. #2284
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
added 3 commits
August 18, 2020 16:04
|
@bharathv : Can you please have a look ? Thanks |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
reviewed
Aug 20, 2020
|
|
||
| Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration(); | ||
|
|
||
| if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){ |
There was a problem hiding this comment.
See how rest of code has space between keyword and brackets as in 'if (' rather than 'if('.... you do this a few times in this PR.
| public final class ReplicationPeerConfigUtil { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(ReplicationPeerConfigUtil.class); | ||
| public static final String HBASE_REPLICATION_PEER_DEFAULT_CONFIG= "hbase.replication.peer.default.config"; |
There was a problem hiding this comment.
Is this 'default' or 'base' configuration for all peers?
There was a problem hiding this comment.
Updated in the latest commit.
| Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration(); | ||
|
|
||
| if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){ | ||
| String[] defaultPeerConfigList = defaultPeerConfigs.split(";"); |
There was a problem hiding this comment.
';' is safe character to use as delimiter for sure? Will never be part of a config value? Are the constraint on peer values at all so you could choose a delimiter that was outside of the constraint set?
There was a problem hiding this comment.
so you could choose a delimiter that was outside of the constraint set
Are there any current constraints regarding the convention on peer values that I can refer to?
I looked around and since there can be multiple values of a particular peer config that are generally delimited by , so I decided to use ; for delimiting different peer configs. But I am happy to change this if it can cause any problems in your opinion. Thanks
There was a problem hiding this comment.
Also Splitter from Guava provides a clean API to do this parsing (couple of lines of code).
There was a problem hiding this comment.
Looks like you missed this, this code can be condensed into
Splitter.on(';').withKeyValueSeparator('=').split(value).trimResults();
| conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_DEFAULT_CONFIG, | ||
| customPeerConfigKey.concat("=").concat(customPeerConfigValue)); | ||
|
|
||
| ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.addDefaultPeerConfigsIfNotPresent(conf,existingReplicationPeerConfig); |
There was a problem hiding this comment.
Long lines? 100chars is max.
|
Is there any place this nice new facility is documented? Thanks. |
I have updated the PR description and also the JIRA about the enhancement that we want to bring as part of this item. Please let me know if you have any questions/concerns will be happy to answer. Thanks |
bharathv
requested changes
Aug 21, 2020
| public final class ReplicationPeerConfigUtil { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(ReplicationPeerConfigUtil.class); | ||
| public static final String HBASE_REPLICATION_PEER_DEFAULT_CONFIG= "hbase.replication.peer.default.config"; |
There was a problem hiding this comment.
This is also getting referenced in other classes for testing, so kept it public.
...ient/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
Show resolved
Hide resolved
| * @param conf Configuration | ||
| * @return true if new configurations was added. | ||
| */ | ||
| public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){ |
There was a problem hiding this comment.
Mind fixing the check-style issues from precommit? Bunch of overflows.
| public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){ | ||
|
|
||
| ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.newBuilder(receivedPeerConfig); | ||
| String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_DEFAULT_CONFIG); |
There was a problem hiding this comment.
nit: get(CONFIG, default)
| /** | ||
| * Helper method to add base peer configs from HBase Configuration to ReplicationPeerConfig | ||
| * @param conf Configuration | ||
| * @return true if new configurations was added. |
There was a problem hiding this comment.
nit: return value javadoc seems wrong.
|
|
||
| ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil. | ||
| addBasePeerConfigsIfNotPresent(this.conf, peerConfig); | ||
| peerStorage.updatePeerConfig(peerId,updatedPeerConfig); |
There was a problem hiding this comment.
I was thinking this happens only in the master code paths. Ex: ReplicationPeerManager#create (for existing peers) or addPeer() for new peers etc. That way the configuration in storage remains consistent.
Doing from the RS code paths (ReplicationPeers) means that if different RS run with different configs it can result in a different final state (depending which RS does this RPC last). Also doing this from HMaster seems logical since this is more like an admin operation whereas RS based codepaths are just consumers of this config.
There was a problem hiding this comment.
Yes, that completely makes sense. Updated in the latest commit.
| Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration(); | ||
|
|
||
| if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){ | ||
| String[] defaultPeerConfigList = defaultPeerConfigs.split(";"); |
There was a problem hiding this comment.
Also Splitter from Guava provides a clean API to do this parsing (couple of lines of code).
| } | ||
|
|
||
| @Test | ||
| public void testDefaultReplicationPeerConfigIsAppliedIfNotAlreadySet(){ |
There was a problem hiding this comment.
Two tests can be merged into one.
There was a problem hiding this comment.
Since they were testing different behaviors I kept them as different, do you think we should still merge them?
| } | ||
|
|
||
| @Test | ||
| public void testDefaultReplicationPeerConfigOverrideIfAlreadySet(){ |
There was a problem hiding this comment.
I think we need more coverage for the following cases.
- Existing peer config gets the config override
- Admin code paths (for getPeerConfig, updatePeerConfig, etc) work well with the overlays (updating an existing / non-existing config etc)
There was a problem hiding this comment.
Added the tests for admin code paths.
The behavior after this patch is that a peer config will only be updated by configuration object if that new configuration was not present in ReplicationPeerConfig. If it was already present then old value is retained and values from configuration object won't make any changes.
|
🎊 +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. |
bharathv
requested changes
Aug 27, 2020
| * @return ReplicationPeerConfig if peer configurations are updated else null. | ||
| */ | ||
| public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf, | ||
| ReplicationPeerConfig receivedPeerConfig){ |
| public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf, | ||
| ReplicationPeerConfig receivedPeerConfig){ | ||
| boolean isPeerConfigChanged = false; | ||
| String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG,null); |
There was a problem hiding this comment.
nit: space between , null
| Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration(); | ||
|
|
||
| if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){ | ||
| String[] defaultPeerConfigList = defaultPeerConfigs.split(";"); |
There was a problem hiding this comment.
Looks like you missed this, this code can be condensed into
Splitter.on(';').withKeyValueSeparator('=').split(value).trimResults();
| } | ||
| } | ||
|
|
||
| return isPeerConfigChanged ? copiedPeerConfigBuilder.build() : null; |
There was a problem hiding this comment.
Why this? Just change the receivedPeerConfig in place?
There was a problem hiding this comment.
Done in latest commit.
| customPeerConfigKey.concat("=").concat(customPeerConfigUpdatedValue)); | ||
|
|
||
| ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil. | ||
| addBasePeerConfigsIfNotPresent(conf,existingReplicationPeerConfig); |
There was a problem hiding this comment.
I lost your comment in the force push, but I think we can merge both these tests into testReplicationBaseConfig() or some such... We cam just add these last two lines in the above test and we don't need all the other boilerplate.
There was a problem hiding this comment.
Sure. Merged the test in latest commit.
|
|
||
| Assert.assertEquals(customPeerConfigUpdatedValue, admin.getReplicationPeerConfig("1"). | ||
| getConfiguration().get(customPeerConfigKey)); | ||
| }finally { |
| ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfig. | ||
| newBuilder(admin.getReplicationPeerConfig("1")). | ||
| putConfiguration(customPeerConfigKey,customPeerConfigUpdatedValue).build(); | ||
| admin.updateReplicationPeerConfig("1", updatedReplicationPeerConfig); |
There was a problem hiding this comment.
Can you also add some tests for deleting a configuration? delete an override, base config should be picked up if it is present
Also, add another peer and make sure it only has the base configuration and not the updated values above.
Also, add a peer, update base configuration, restart cluster, new base should be picked up (our target usecase essentially).
There was a problem hiding this comment.
Looks like hbase does not provide the support for deleting a peer configuration, we can only update the value after a configuration is added. But I have added the test case for the scenario that new base config gets picked up after hbase restart.
|
🎊 +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. |
bharathv
changed the title
bharathv
approved these changes
Aug 28, 2020
| */ | ||
| public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf, | ||
| ReplicationPeerConfig receivedPeerConfig) { | ||
| String basePeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG, null); |
There was a problem hiding this comment.
nit: Use the default as empty string? That way we can avoid null check in if, not a big deal, just to be consistent with other similar usages.
| ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig. | ||
| newBuilder(receivedPeerConfig); | ||
| Map<String,String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration(); | ||
|
|
There was a problem hiding this comment.
nit: Remove multiple extraneous new lines in this method.
| String configValue = entry.getValue(); | ||
| // Only override if base config does not exist in existing peer configs | ||
| if (!receivedPeerConfigMap.containsKey(configName)) { | ||
| copiedPeerConfigBuilder.putConfiguration(configName,configValue); |
There was a problem hiding this comment.
nit:, configValue (space)
| return; | ||
| } | ||
| ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil. | ||
| addBasePeerConfigsIfNotPresent(conf,peerConfig); |
There was a problem hiding this comment.
nit: config, peerConfig (space)
| } | ||
| ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil. | ||
| addBasePeerConfigsIfNotPresent(conf,peerConfig); | ||
| peerConfig = updatedPeerConfig; |
There was a problem hiding this comment.
merge into a single line.
peerConfig = ReplicatinPeerConfigUtil..addPeer..IfNotPresent()
|
|
||
| ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil. | ||
| addBasePeerConfigsIfNotPresent(conf,peerConfig); | ||
| peerStorage.updatePeerConfig(peerId,updatedPeerConfig); |
There was a problem hiding this comment.
nit: , updatedPeerConfig (space).
| for (String peerId : peerStorage.listPeerIds()) { | ||
| ReplicationPeerConfig peerConfig = peerStorage.getPeerConfig(peerId); | ||
|
|
||
| ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil. |
There was a problem hiding this comment.
peerConfig = ReplicationpeerConfigUtil.....() (avoid unnecessary temp variable)
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bharathv
reviewed
Sep 1, 2020
There was a problem hiding this comment.
@saintstack you have more comments on this one or is this good to go?
|
@saintstack Can you please have a look at this patch whenever you get a chance? I have addressed all your comments. Thanks |
|
@saintstack sorry to bother you, just checking to see if you have any pending comments. Will go ahead and merge this (tomorrow) if you are busy and won't be able to take another pass. Ankit can do a follow up if you have any comments later. Thanks. |
bharathv
pushed a commit
that referenced
this pull request
Sep 10, 2020
…e.xml for all replication peers. (#2284) Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…e.xml for all replication peers. (apache#2284) Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
virajjasani
pushed a commit
that referenced
this pull request
Dec 22, 2020
…e.xml for all replication peers. (#2284) Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…cation plugin: 1) Revert "HBASE-24743 Reject to add a peer which replicate to itself earlier (apache#2124)" This reverts commit 70ab0dc. 2)Revert "HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers. (apache#2284)" This reverts commit 7df1b92. Change-Id: I8d5e66a9073caf61e744a19425c350c49f4b7072
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.
JIRA: https://issues.apache.org/jira/browse/HBASE-24764
JIRA Description:
Today, if a user needs to apply some common base peer configs to all the replication peers on the cluster, the only way is to execute update_peer_config via CLI which requires manual intervention and can be tedious in case of large deployment fleet.
As part of this JIRA, we plan to add the support to have base replication peer configs as part of hbase-site.xml like hbase.replication.peer.base.config="k1=v1;k2=v2.." which can be easily updated and applied as part of a rolling restart.
Example below:
This property will be empty by default, but user can override to have base configs in place.
The final peer configuration would be a merge of whatever is currently present or what users override during the peer creation/update (if any) + this newly added base config.
Related Jira: https://issues.apache.org/jira/browse/HBASE-17543. HBASE-17543 added the support to add the WALEntryFilters to default endpoint via peer configuration.
By this new Jira we are extending the support to update peer configs via hbase-site.xml and hence WalEntryFilters or any other peer property can be applied just by rolling restart.
Branch-1 PR: #2327