EdColeman commented on code in PR #2569:
URL: https://github.com/apache/accumulo/pull/2569#discussion_r850887413
##########
server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java:
##########
@@ -18,45 +18,43 @@
*/
package org.apache.accumulo.server.util;
-import static java.nio.charset.StandardCharsets.UTF_8;
+import java.util.Collection;
+import java.util.Map;
-import org.apache.accumulo.core.Constants;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.data.AbstractId;
import org.apache.accumulo.core.data.NamespaceId;
-import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
-import org.apache.accumulo.fate.zookeeper.ZooUtil.NodeExistsPolicy;
-import org.apache.accumulo.fate.zookeeper.ZooUtil.NodeMissingPolicy;
import org.apache.accumulo.server.ServerContext;
-import org.apache.zookeeper.KeeperException;
+import org.apache.accumulo.server.conf.store.PropCacheKey;
-public class NamespacePropUtil {
- public static boolean setNamespaceProperty(ServerContext context,
NamespaceId namespaceId,
- String property, String value) throws KeeperException,
InterruptedException {
- if (!Property.isTablePropertyValid(property, value))
- return false;
+public class NamespacePropUtil implements PropUtil {
- ZooReaderWriter zoo = context.getZooReaderWriter();
+ private NamespaceId namespaceId;
- // create the zk node for per-namespace properties for this namespace if
it doesn't already
- // exist
- String zkNamespacePath = getPath(context, namespaceId);
- zoo.putPersistentData(zkNamespacePath, new byte[0], NodeExistsPolicy.SKIP);
+ private NamespacePropUtil() {}
- // create the zk node for this property and set it's data to the specified
value
- String zPath = zkNamespacePath + "/" + property;
- zoo.putPersistentData(zPath, value.getBytes(UTF_8),
NodeExistsPolicy.OVERWRITE);
-
- return true;
+ public static NamespacePropUtil factory() {
+ return new NamespacePropUtil();
}
- public static void removeNamespaceProperty(ServerContext context,
NamespaceId namespaceId,
- String property) throws InterruptedException, KeeperException {
- String zPath = getPath(context, namespaceId) + "/" + property;
- context.getZooReaderWriter().recursiveDelete(zPath,
NodeMissingPolicy.SKIP);
+ @Override
+ public void setProperties(ServerContext context, AbstractId<?> namespaceId,
+ Map<String,String> properties) {
+ for (Map.Entry<String,String> prop : properties.entrySet()) {
+ // TODO reconcile with TablePropUtil on invalid, this throws exception,
table ignores
Review Comment:
Same response for TablePropUtil
Yes - the current code is consistent and preservers the current behavior -
but it seems odd that the current behavior is different between the two classes
- on a table it returns a boolean and setting and invalid property returns
false. For a namespace, it returns void, but will throw an exception.
Both are valid responses, but seems like we could pick one way or the other
- or I'm missing something.
So, right or wrong, this code matches the current code behavior. I do not
have a preference for one method over the other. At some point it may be worth
a look, especially if it simplifies things, but for this PR it is not required
and the TODO points to something that might be of interest that is not obvious
unless someone is specifically comparing the two classes / operations.
I think a long term solution would be to pick one, and then it is likely
that the NamespacePropUtil and TablePropUtil might be able to be collapsed into
a single class. But that does not need to occur in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
