Skip to content

HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers. #2284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bharathv merged 14 commits into from
Sep 10, 2020
Merged

HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers. #2284

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8510b99
HBASE-24764: Add support of adding default peer configs via hbase-sit…
Aug 18, 2020
4b83e2c
HBASE-24764: Moving logic to ReplicationPeerConfigUtil
Aug 19, 2020
b50ceab
Removing extra line
Aug 19, 2020
283a180
HBASE-24764: Addressing review comments
Aug 21, 2020
68b769e
HBASE-24764: Updating tests
Aug 21, 2020
d815e13
HBASE-24764: Fixing formatting issues
Aug 21, 2020
560154a
Review comments
Aug 25, 2020
6d5ac0f
HBASE-24764: Review comments and adding more tests
Aug 26, 2020
df48cb0
HBASE-24764: Remove extra lines
Aug 26, 2020
a65f3e4
HBASE-24764: Modifying test description
Aug 27, 2020
e2f08b0
HBASE-24764 : Review comments and adding more tests
Aug 27, 2020
20e3350
HBASE-24764 : Review Comments
Aug 27, 2020
9147c93
Remvoing shaded imports
Aug 27, 2020
eb9c543
HBASE-24764: Addressing review comments
Aug 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions ...t/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
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";

@bharathv bharathv Aug 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it private?

@ankitjain64 ankitjain64 Aug 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also getting referenced in other classes for testing, so kept it public.

@saintstack saintstack Aug 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this 'default' or 'base' configuration for all peers?

@ankitjain64 ankitjain64 Aug 21, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in the latest commit.


private ReplicationPeerConfigUtil() {}

Expand Down Expand Up @@ -450,6 +451,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}

/**
Comment thread
bharathv marked this conversation as resolved.
Sample Configuration
<property>
<name>hbase.replication.peer.default.configs</name>
<value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.xxx.custom_property=123</value>
</property>
*/

/**
* Helper method to add default peer configs from HBase Configuration to ReplicationPeerConfig
* @param conf Configuration
* @return true if new configurations was added.

@bharathv bharathv Aug 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: return value javadoc seems wrong.

*/
public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){

@bharathv bharathv Aug 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind fixing the check-style issues from precommit? Bunch of overflows.


ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.newBuilder(receivedPeerConfig);
String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_DEFAULT_CONFIG);

@bharathv bharathv Aug 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: get(CONFIG, default)


@bharathv bharathv Aug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Remove multiple extraneous new lines in this method.

Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();

if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){

@saintstack saintstack Aug 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

String[] defaultPeerConfigList = defaultPeerConfigs.split(";");

@saintstack saintstack Aug 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

';' 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?

@ankitjain64 ankitjain64 Aug 21, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bharathv bharathv Aug 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also Splitter from Guava provides a clean API to do this parsing (couple of lines of code).

@bharathv bharathv Aug 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you missed this, this code can be condensed into

Splitter.on(';').withKeyValueSeparator('=').split(value).trimResults();


for(String defaultPeerConfig : defaultPeerConfigList){
String[] configSplit = defaultPeerConfig.split("=");
if(configSplit != null && configSplit.length == 2){
String configName = configSplit[0];
String configValue = configSplit[1];

// Only override if default property does not exist in existing peer configs or its value is different.
if(!peerConfigurations.containsKey(configName) || !peerConfigurations.get(configName).equalsIgnoreCase(configValue)){
copiedPeerConfigBuilder.putConfiguration(configName,configValue);
}
}
}
}
return copiedPeerConfigBuilder.build();
}

public static ReplicationPeerConfig appendExcludeTableCFsToReplicationPeerConfig(
Map<TableName, List<String>> excludeTableCfs, ReplicationPeerConfig peerConfig)
throws ReplicationException {
Expand Down
7 changes: 6 additions & 1 deletion hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.client.replication.ReplicationPeerConfigUtil;
import org.apache.hadoop.hbase.replication.ReplicationPeer.PeerState;
import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
import org.apache.yetus.audience.InterfaceAudience;
Expand Down Expand Up @@ -144,7 +145,11 @@ private ReplicationPeerImpl createPeer(String peerId) throws ReplicationExceptio
SyncReplicationState syncReplicationState = peerStorage.getPeerSyncReplicationState(peerId);
SyncReplicationState newSyncReplicationState =
peerStorage.getPeerNewSyncReplicationState(peerId);

ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil.addDefaultPeerConfigsIfNotPresent(this.conf, peerConfig);
peerStorage.updatePeerConfig(peerId,updatedPeerConfig);

@bharathv bharathv Aug 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ankitjain64 ankitjain64 Aug 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that completely makes sense. Updated in the latest commit.


return new ReplicationPeerImpl(ReplicationUtils.getPeerClusterConfiguration(peerConfig, conf),
peerId, peerConfig, enabled, syncReplicationState, newSyncReplicationState);
peerId, updatedPeerConfig, enabled, syncReplicationState, newSyncReplicationState);
}
}
39 changes: 39 additions & 0 deletions ...ation/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,12 @@
import java.util.Random;
import java.util.Set;
import java.util.stream.Stream;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseZKTestingUtility;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.replication.ReplicationPeerConfigUtil;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.testclassification.ReplicationTests;
import org.apache.hadoop.hbase.zookeeper.ZKUtil;
Expand Down Expand Up @@ -215,4 +218,40 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}

@Test
public void testDefaultReplicationPeerConfigIsAppliedIfNotAlreadySet(){

@bharathv bharathv Aug 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two tests can be merged into one.

@ankitjain64 ankitjain64 Aug 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since they were testing different behaviors I kept them as different, do you think we should still merge them?

String customPeerConfigKey = "hbase.xxx.custom_config";
String customPeerConfigValue = "test";

ReplicationPeerConfig existingReplicationPeerConfig = getConfig(1);

// custom config not present
assertEquals(existingReplicationPeerConfig.getConfiguration().get(customPeerConfigKey), null);

Configuration conf = UTIL.getConfiguration();
conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_DEFAULT_CONFIG,
customPeerConfigKey.concat("=").concat(customPeerConfigValue));

ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.addDefaultPeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);

@saintstack saintstack Aug 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long lines? 100chars is max.

assertEquals(customPeerConfigValue, updatedReplicationPeerConfig.getConfiguration().get(customPeerConfigKey));
}

@Test
public void testDefaultReplicationPeerConfigOverrideIfAlreadySet(){

@bharathv bharathv Aug 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@ankitjain64 ankitjain64 Aug 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


String customPeerConfigKey = "hbase.xxx.custom_config";
String customPeerConfigValue = "test";
String customPeerConfigUpdatedValue = "test_updated";

ReplicationPeerConfig existingReplicationPeerConfig = ReplicationPeerConfig.newBuilder(getConfig(1))
.putConfiguration(customPeerConfigKey,customPeerConfigValue).build();

Configuration conf = UTIL.getConfiguration();
conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_DEFAULT_CONFIG,
customPeerConfigKey.concat("=").concat(customPeerConfigUpdatedValue));

ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.addDefaultPeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
assertEquals(customPeerConfigUpdatedValue, updatedReplicationPeerConfig.getConfiguration().get(customPeerConfigKey));
}
}
3 changes: 3 additions & 0 deletions ...rver/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.hadoop.hbase.HBaseConfiguration;
import org.apache.hadoop.hbase.ServerName;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.replication.ReplicationPeerConfigUtil;
import org.apache.hadoop.hbase.replication.BaseReplicationEndpoint;
import org.apache.hadoop.hbase.replication.ReplicationEndpoint;
import org.apache.hadoop.hbase.replication.ReplicationException;
Expand Down Expand Up @@ -119,6 +120,8 @@ void preAddPeer(String peerId, ReplicationPeerConfig peerConfig)
if (peerId.contains("-")) {
throw new DoNotRetryIOException("Found invalid peer name: " + peerId);
}
ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil.addDefaultPeerConfigsIfNotPresent(conf,peerConfig);
peerConfig = updatedPeerConfig;
checkPeerConfig(peerConfig);
if (peerConfig.isSyncReplication()) {
checkSyncReplicationPeerConfigConflict(peerConfig);
Expand Down