EdColeman commented on code in PR #2569:
URL: https://github.com/apache/accumulo/pull/2569#discussion_r850886305
##########
server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java:
##########
@@ -18,48 +18,47 @@
*/
package org.apache.accumulo.server.util;
-import static java.nio.charset.StandardCharsets.UTF_8;
+import java.util.Collection;
+import java.util.HashMap;
+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.TableId;
-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;
+import org.apache.accumulo.server.conf.store.PropStoreException;
-public class TablePropUtil {
+public class TablePropUtil implements PropUtil {
- public static boolean setTableProperty(ServerContext context, TableId
tableId, String property,
- String value) throws KeeperException, InterruptedException {
- return setTableProperty(context.getZooReaderWriter(),
context.getZooKeeperRoot(), tableId,
- property, value);
- }
-
- public static boolean setTableProperty(ZooReaderWriter zoo, String zkRoot,
TableId tableId,
- String property, String value) throws KeeperException,
InterruptedException {
- if (!Property.isTablePropertyValid(property, value))
- return false;
+ private TablePropUtil() {}
- // create the zk node for per-table properties for this table if it
doesn't already exist
- String zkTablePath = getTablePath(zkRoot, tableId);
- zoo.putPersistentData(zkTablePath, new byte[0], NodeExistsPolicy.SKIP);
+ public static TablePropUtil factory() {
+ return new TablePropUtil();
+ }
- // create the zk node for this property and set it's data to the specified
value
- String zPath = zkTablePath + "/" + property;
- zoo.putPersistentData(zPath, value.getBytes(UTF_8),
NodeExistsPolicy.OVERWRITE);
+ /**
+ * Helper method to set provided properties for the provided table.
+ *
+ * @throws PropStoreException
+ * if an underlying exception (KeeperException,
InterruptException) or other failure to
+ * read properties from the cache / backend store
+ */
+ @Override
+ public void setProperties(ServerContext context, AbstractId<?> tableId,
+ Map<String,String> props) {
+ Map<String,String> tempProps = new HashMap<>(props);
+ // TODO reconcile with NamespacePropUtil on invalid, this ignores,
namespace throws exception
Review Comment:
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]
