[
https://issues.apache.org/jira/browse/HBASE-3909?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247113#comment-13247113
]
Uma Maheswara Rao G commented on HBASE-3909:
--------------------------------------------
@Subbu,
I just reviewed this patch. Feedback as follows.
| 1) updateClusterConfig and createClusterConfig are looks like mostly
duplicate code for me. Can we reuse the code or can change the API, such that
it can handle both.
{code}
if (ZKUtil.checkExists(watcher,
+ getConfigNodePath(configKey)) > 0) {
+ LOG.info("Cluster configuration node exist for Config Key = "
+ + configKey + " Updating the cluster config node");
+ ZKUtil.setData(watcher, getConfigNodePath(configKey),
Bytes.toBytes(configValue));
+ } else {
+ ZKUtil.createSetData(watcher, getConfigNodePath(configKey),
+ Bytes.toBytes(configValue));
+ }
{code}
| 2) Looks your are using wrong code formatter. intendation should be 2 spaces.
{code}
try {
+ if (ZKUtil.checkExists(watcher,
{code}
| 3) Normally we used to put the licence header at top of the file.
{code}
+package org.apache.hadoop.hbase.zookeeper;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.Abortable;
+import org.apache.hadoop.hbase.HBaseInMemoryConfiguration;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.zookeeper.KeeperException;
+import org.codehaus.jackson.map.ObjectMapper;
+
+import java.io.IOException;
+import java.io.StringWriter;
+import java.util.Map;
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
{code}
| 4) configuration.dumpConfiguration(configuration, outWriter); , this api is
static in configuration , you can call directly with class.
| 5) getMaster in HBaseAdmin seems like deprecated
{code}
return getMaster().updateConfig(configKey, configValue);
{code}
| 6) please add the javadoc cleanly
{code}
/**
+ *
+ * @param configKey
+ * @param configValue
+ * @return
+ */
{code}
| 7) a) Call back comes from ZK for dataChangeEvent,
b) when you are processing the event at Hmaster/Regionserver to refresh the
configurations.
c) Now unfortunately ZK down/nw fluctuation from ZK to Server.
d) It may get data as null. And refreshClusterConfigMap will just ignore the
dataupdation in memory.
Then this node may continue with old configs right? when this will get
updated again?
seems to be a bug here. no?
You have used ZKUtil#getChildDataAndWatchForNewChildren, it can throw NPE, I
Just filed JIRA HBASE-5722
| 8) Looks you are creating znodes for all the configuration items.
I feel, we should not allow user to change all the configuration items. (
like some configurations we may not be able to change at runtime, may be zk url
etc..).
So, while creating znodes it self we can create only for mutable config
items in ZK. If user tries to update any immutable config items, you can
directly emit the warning.
Am i missing some thing here?
| 9) I think we may need to add the validation checks for the mutable
configurations from admin. Since we are encouraging the users to update configs
dynamically, if user sets some wrong values, it may change directly and system
may go immediatly to unstable state. may be dangerous. no?
| 10) Looks we are handling nodeDeleted event. When exactly this event can come?
I think we added updateConfig api in HBase admin. How can we remove
element with this? Is this event really will occur?
| 11) Final question is that, we are claiming hadoop can run in commodity
hardwares, in that case some configuration items can be different in each
machine. for example: 'hbase.regionserver.handler.count'..etc
| 12)createClusterConfig creating JSonConfiguration from the Configuration and
adding them into HBaseInMemoryConfiguration.
Can't we create update the HBaseInMemoryConfiguration from input
Configuration.
Please correct me if I missunderstand your design. Great work thanks a lot.
Thanks
Uma
> Add dynamic config
> ------------------
>
> Key: HBASE-3909
> URL: https://issues.apache.org/jira/browse/HBASE-3909
> Project: HBase
> Issue Type: Bug
> Reporter: stack
> Fix For: 0.96.0
>
> Attachments: 3909-v1.patch, 3909.v1
>
>
> I'm sure this issue exists already, at least as part of the discussion around
> making online schema edits possible, but no hard this having its own issue.
> Ted started a conversation on this topic up on dev and Todd suggested we
> lookd at how Hadoop did it over in HADOOP-7001
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
