This is an automated email from the ASF dual-hosted git repository.
reddycharan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 2576afd Add validateConf to AbstractDNSToSwitchMapping
2576afd is described below
commit 2576afdbe460e76047999725bd5988486ccf378e
Author: Charan Reddy Guttapalem <[email protected]>
AuthorDate: Tue Mar 5 10:27:04 2019 -0800
Add validateConf to AbstractDNSToSwitchMapping
Descriptions of the changes in this PR:
- when setConf of AbstractDNSToSwitchMapping is called it
should do sanity checking of the conf/env. and throw
RuntimeException if things are not valid.
- For RawScriptBasedMapping.validateConf, try executing
the script with no arguments for sanity check purpose.
Here it is expected that running script with no arguments
would do sanity check of the script and the env.
(there are 2 commits in this PR, but this PR is meant for the second commit
and there is other PR for the first commit)
Reviewers: Sijie Guo <[email protected]>
This closes #1965 from reddycharan/sanitycheckmappingconf
---
.../RackawareEnsemblePlacementPolicyImpl.java | 4 +-
.../bookkeeper/net/AbstractDNSToSwitchMapping.java | 7 +++
.../apache/bookkeeper/net/ScriptBasedMapping.java | 35 ++++++++++---
...ackawareEnsemblePlacementPolicyUsingScript.java | 58 ++++++++++++++++++++++
4 files changed, 96 insertions(+), 8 deletions(-)
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
index 4cbe25b..ed35d49 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
@@ -387,8 +387,8 @@ public class RackawareEnsemblePlacementPolicyImpl extends
TopologyAwareEnsembleP
}
} catch (RuntimeException re) {
if (!conf.getEnforceMinNumRacksPerWriteQuorum()) {
- LOG.info("Failed to initialize DNS Resolver {}, used
default subnet resolver : {}", dnsResolverName,
- re, re.getMessage());
+ LOG.error("Failed to initialize DNS Resolver {}, used
default subnet resolver : {}",
+ dnsResolverName, re, re.getMessage());
dnsResolver = new DefaultResolver(() ->
this.getDefaultRack());
} else {
/*
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/AbstractDNSToSwitchMapping.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/AbstractDNSToSwitchMapping.java
index a19cc62..77e64c0 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/AbstractDNSToSwitchMapping.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/AbstractDNSToSwitchMapping.java
@@ -65,6 +65,7 @@ public abstract class AbstractDNSToSwitchMapping implements
DNSToSwitchMapping,
public void setConf(Configuration conf) {
this.conf = conf;
+ validateConf();
}
/**
@@ -138,4 +139,10 @@ public abstract class AbstractDNSToSwitchMapping
implements DNSToSwitchMapping,
&& ((AbstractDNSToSwitchMapping) mapping).isSingleSwitch();
}
+ /**
+ * when setConf is called it should do sanity checking of the conf/env. and
+ * throw RuntimeException if things are not valid.
+ */
+ protected void validateConf() {
+ }
}
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/ScriptBasedMapping.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/ScriptBasedMapping.java
index 0ef92ef..890bcfc 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/ScriptBasedMapping.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/ScriptBasedMapping.java
@@ -132,13 +132,19 @@ public final class ScriptBasedMapping extends
CachedDNSToSwitchMapping {
private int maxArgs; //max hostnames per call of the script
private static final Logger LOG =
LoggerFactory.getLogger(ScriptBasedMapping.class);
- /**
- * Set the configuration and extract the configuration parameters of
interest.
- * @param conf the new configuration
+ /*
+ * extract 'scriptName' and 'maxArgs' parameters from the conf and
throw
+ * RuntimeException if 'scriptName' is null. Also for sanity check
+ * purpose try executing the script with no arguments. Here it is
+ * expected that running script with no arguments would do sanity check
+ * of the script and the env, and return successfully if script and
env.
+ * are valid. If sanity check of the script with no argument fails then
+ * throw RuntimeException.
+ *
*/
@Override
- public void setConf(Configuration conf) {
- super.setConf(conf);
+ protected void validateConf() {
+ Configuration conf = getConf();
if (conf != null) {
String scriptNameConfValue =
conf.getString(SCRIPT_FILENAME_KEY);
if (StringUtils.isNotBlank(scriptNameConfValue)) {
@@ -154,7 +160,24 @@ public final class ScriptBasedMapping extends
CachedDNSToSwitchMapping {
}
if (null == scriptName) {
- throw new RuntimeException("No network topology script is
found when using script based DNS resolver.");
+ throw new RuntimeException("No network topology script is
found when using script"
+ + " based DNS resolver.");
+ } else {
+ File dir = null;
+ String userDir;
+ if ((userDir = System.getProperty("user.dir")) != null) {
+ dir = new File(userDir);
+ }
+ String[] execString = { this.scriptName };
+ ShellCommandExecutor s = new ShellCommandExecutor(execString,
dir);
+ try {
+ s.execute();
+ } catch (Exception e) {
+ LOG.error("Conf validation failed. Got exception for
sanity check of script: " + this.scriptName,
+ e);
+ throw new RuntimeException(
+ "Conf validation failed. Got exception for sanity
check of script: " + this.scriptName, e);
+ }
}
}
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicyUsingScript.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicyUsingScript.java
index fc3caaf..f90f45d 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicyUsingScript.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicyUsingScript.java
@@ -403,6 +403,64 @@ public class
TestRackawareEnsemblePlacementPolicyUsingScript {
repp.uninitalize();
}
+ @Test
+ public void testIfValidateConfFails() throws Exception {
+ ignoreTestIfItIsWindowsOS();
+ repp.uninitalize();
+
+ ClientConfiguration newConf = new ClientConfiguration();
+ newConf.setProperty(REPP_DNS_RESOLVER_CLASS,
ScriptBasedMapping.class.getName());
+ /*
+ * this script, exits with error value if no argument is passed to it.
+ * So mapping.validateConf will fail.
+ */
+
newConf.setProperty(CommonConfigurationKeys.NET_TOPOLOGY_SCRIPT_FILE_NAME_KEY,
+ "src/test/resources/networkmappingscriptwithargs.sh");
+ timer = new HashedWheelTimer(new
ThreadFactoryBuilder().setNameFormat("TestTimer-%d").build(),
+ newConf.getTimeoutTimerTickDurationMs(),
TimeUnit.MILLISECONDS, newConf.getTimeoutTimerNumTicks());
+
+ repp = new RackawareEnsemblePlacementPolicy();
+ repp.initialize(newConf, Optional.<DNSToSwitchMapping> empty(), timer,
DISABLE_ALL, NullStatsLogger.INSTANCE);
+
+ repp.uninitalize();
+ repp = new RackawareEnsemblePlacementPolicy();
+ try {
+ repp.initialize(newConf, Optional.<DNSToSwitchMapping> empty(),
timer, DISABLE_ALL,
+ NullStatsLogger.INSTANCE);
+ } catch (RuntimeException re) {
+ fail("EnforceMinNumRacksPerWriteQuorum is not set, so
repp.initialize should succeed"
+ + " even if mapping.validateConf fails");
+ }
+
+ newConf.setEnforceMinNumRacksPerWriteQuorum(true);
+ repp.uninitalize();
+ repp = new RackawareEnsemblePlacementPolicy();
+ try {
+ repp.initialize(newConf, Optional.<DNSToSwitchMapping> empty(),
timer, DISABLE_ALL,
+ NullStatsLogger.INSTANCE);
+ fail("EnforceMinNumRacksPerWriteQuorum is set, so repp.initialize
should fail"
+ + " if mapping.validateConf fails");
+ } catch (RuntimeException re) {
+
+ }
+
+ /*
+ * this script returns successfully even if no argument is passed to
it.
+ * So mapping.validateConf will succeed.
+ */
+
newConf.setProperty(CommonConfigurationKeys.NET_TOPOLOGY_SCRIPT_FILE_NAME_KEY,
+ "src/test/resources/networkmappingscript.sh");
+ repp.uninitalize();
+ repp = new RackawareEnsemblePlacementPolicy();
+ try {
+ repp.initialize(newConf, Optional.<DNSToSwitchMapping> empty(),
timer, DISABLE_ALL,
+ NullStatsLogger.INSTANCE);
+ } catch (RuntimeException re) {
+ fail("EnforceMinNumRacksPerWriteQuorum is set, and
mapping.validateConf succeeds."
+ + " So repp.initialize should succeed");
+ }
+ }
+
private int getNumCoveredWriteQuorums(List<BookieSocketAddress> ensemble,
int writeQuorumSize)
throws Exception {
int ensembleSize = ensemble.size();
