ctubbsii commented on code in PR #4996:
URL: https://github.com/apache/accumulo/pull/4996#discussion_r1813182170
##########
server/base/src/main/java/org/apache/accumulo/server/init/ZooKeeperInitializer.java:
##########
@@ -116,9 +117,10 @@ void initialize(final ServerContext context, final boolean
clearInstanceName,
ZooUtil.NodeExistsPolicy.FAIL);
TableManager.prepareNewNamespaceState(context, Namespace.DEFAULT.id(),
Namespace.DEFAULT.name(),
- ZooUtil.NodeExistsPolicy.FAIL);
+ ZooUtil.NodeExistsPolicy.OVERWRITE);
Review Comment:
I'm curious why these were changed to OVERWRITE instead of FAIL. During
initialization, the node for the namespaceId shouldn't exist yet, so it should
fail if it already exists, because that's unexpected. The namespaces node
should exist (because it's created up on line 116), but the individual
namespace nodes for the two initial namespaces should not yet exist, and init
should fail if they happen to already exist.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/namespace/rename/RenameNamespace.java:
##########
@@ -63,19 +65,23 @@ public Repo<Manager> call(FateId fateId, Manager manager)
throws Exception {
Utils.checkNamespaceDoesNotExist(manager.getContext(), newName,
namespaceId,
TableOperation.RENAME);
- final String tap = manager.getZooKeeperRoot() + Constants.ZNAMESPACES +
"/" + namespaceId
- + Constants.ZNAMESPACE_NAME;
+ final String tap = manager.getZooKeeperRoot() + Constants.ZNAMESPACES;
+
+ final String currentNamespaceName =
+ Namespaces.getNamespaceName(manager.getContext(), namespaceId);
+ final byte[] updatedNamespaceMap =
Review Comment:
If the check for the ID already existing is done in the mutateExisting, then
the check to see if it exists already in line 65 could be removed in favor of
the check inside the mutateExisting. However, if the goal is to preserve the
same exceptions that are being thrown by `Utils.checkNamespaceDoesNotExist`,
which I believe throws a checked exception, then some cleverness will need to
be done to throw a custom unchecked (RTE) exception inside the mutateExisting
lambda, and then outside, the mutateExisting, it would need to be converted
back to the same checked exception that the `Utils` method would have thrown.
I think there's a few more places where the
`Utils.checkNamespaceDoesNotExist` method is used that could also be replaced
in this same way. Then, that utility method could just be deleted.
##########
server/base/src/main/java/org/apache/accumulo/server/init/ZooKeeperInitializer.java:
##########
@@ -116,9 +117,10 @@ void initialize(final ServerContext context, final boolean
clearInstanceName,
ZooUtil.NodeExistsPolicy.FAIL);
TableManager.prepareNewNamespaceState(context, Namespace.DEFAULT.id(),
Namespace.DEFAULT.name(),
- ZooUtil.NodeExistsPolicy.FAIL);
+ ZooUtil.NodeExistsPolicy.OVERWRITE);
TableManager.prepareNewNamespaceState(context, Namespace.ACCUMULO.id(),
- Namespace.ACCUMULO.name(), ZooUtil.NodeExistsPolicy.FAIL);
+ Namespace.ACCUMULO.name(), ZooUtil.NodeExistsPolicy.OVERWRITE);
+ NamespaceMapping.initializeNamespaceMap(zoo, zkInstanceRoot +
Constants.ZNAMESPACES);
Review Comment:
This line could be moved up to and combined with the one on line 116 that
initializes the node to an empty byte array. It could be initialized directly
with the initial mapping instead, in one line. It should also use FAIL policy,
because it shouldn't yet exist up there at line 116.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.clientImpl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Collections.emptySortedMap;
+
+import java.lang.reflect.Type;
+import java.util.Collections;
+import java.util.Map;
+import java.util.SortedMap;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.data.NamespaceId;
+import org.apache.accumulo.core.fate.zookeeper.ZooCache;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.reflect.TypeToken;
+import com.google.gson.Gson;
+
+public class NamespaceMapping {
+ private static final Gson gson = new Gson();
+ private final ClientContext context;
+ private volatile SortedMap<NamespaceId,String> currentNamespaceMap =
emptySortedMap();
+ private volatile SortedMap<String,NamespaceId> currentNamespaceReverseMap =
emptySortedMap();
+ private volatile long lastMzxid;
+
+ public NamespaceMapping(ClientContext context) {
+ this.context = context;
+ }
+
+ public static void initializeNamespaceMap(ZooReaderWriter zoo, String zPath)
+ throws InterruptedException, KeeperException {
+ Map<String,String> map = Map.of(Namespace.DEFAULT.id().canonical(),
Namespace.DEFAULT.name(),
+ Namespace.ACCUMULO.id().canonical(), Namespace.ACCUMULO.name());
+ zoo.putPersistentData(zPath, serialize(map),
ZooUtil.NodeExistsPolicy.OVERWRITE);
+ }
+
+ public static byte[] writeNamespaceToMap(ZooReaderWriter zoo, String zPath,
+ NamespaceId namespaceId, String namespaceName) throws
InterruptedException, KeeperException {
+ byte[] updatedMap = new byte[0];
+ if (!Namespace.DEFAULT.id().equals(namespaceId)
+ && !Namespace.ACCUMULO.id().equals(namespaceId)) {
+ byte[] data = zoo.getData(zPath);
+ Map<String,String> namespaceMap = deserialize(data);
+ namespaceMap.put(namespaceId.canonical(), namespaceName);
+ updatedMap = serialize(namespaceMap);
+ }
+ return updatedMap;
+ }
+
+ public static byte[] serialize(Map<String,String> map) {
+ var sortedMap =
ImmutableSortedMap.<String,String>naturalOrder().putAll(map).build();
+ String jsonData = gson.toJson(sortedMap);
+ return jsonData.getBytes(UTF_8);
+ }
+
+ public static Map<String,String> deserialize(byte[] data) {
+ if (data == null || data.length == 0) {
+ return Collections.emptyMap();
+ }
Review Comment:
throwing an AssertionError would probably be okay for this if block, since
it's never expected to happen and would indicate a programming error on our
part (or if the user messed with the ZK storage, I suppose).
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.clientImpl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Collections.emptySortedMap;
+
+import java.lang.reflect.Type;
+import java.util.Collections;
+import java.util.Map;
+import java.util.SortedMap;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.data.NamespaceId;
+import org.apache.accumulo.core.fate.zookeeper.ZooCache;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.reflect.TypeToken;
+import com.google.gson.Gson;
+
+public class NamespaceMapping {
+ private static final Gson gson = new Gson();
+ private final ClientContext context;
+ private volatile SortedMap<NamespaceId,String> currentNamespaceMap =
emptySortedMap();
+ private volatile SortedMap<String,NamespaceId> currentNamespaceReverseMap =
emptySortedMap();
+ private volatile long lastMzxid;
+
+ public NamespaceMapping(ClientContext context) {
+ this.context = context;
+ }
+
+ public static void initializeNamespaceMap(ZooReaderWriter zoo, String zPath)
+ throws InterruptedException, KeeperException {
+ Map<String,String> map = Map.of(Namespace.DEFAULT.id().canonical(),
Namespace.DEFAULT.name(),
+ Namespace.ACCUMULO.id().canonical(), Namespace.ACCUMULO.name());
+ zoo.putPersistentData(zPath, serialize(map),
ZooUtil.NodeExistsPolicy.OVERWRITE);
+ }
+
+ public static byte[] writeNamespaceToMap(ZooReaderWriter zoo, String zPath,
+ NamespaceId namespaceId, String namespaceName) throws
InterruptedException, KeeperException {
+ byte[] updatedMap = new byte[0];
+ if (!Namespace.DEFAULT.id().equals(namespaceId)
Review Comment:
A comment here would help explain why these are excluded (specifically,
because init doesn't use this method to write code, and they should never have
a name change after init creates them, so they'll never be updated).
```suggestion
// the built-in namespaces were already added during init or upgrade and
can't be changed
if (!Namespace.DEFAULT.id().equals(namespaceId)
```
##########
server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java:
##########
@@ -71,11 +72,11 @@ public static void prepareNewNamespaceState(ZooReaderWriter
zoo, final PropStore
InstanceId instanceId, NamespaceId namespaceId, String namespace,
NodeExistsPolicy existsPolicy) throws KeeperException,
InterruptedException {
log.debug("Creating ZooKeeper entries for new namespace {} (ID: {})",
namespace, namespaceId);
- String zPath = Constants.ZROOT + "/" + instanceId + Constants.ZNAMESPACES
+ "/" + namespaceId;
+ String zPath = Constants.ZROOT + "/" + instanceId + Constants.ZNAMESPACES;
- zoo.putPersistentData(zPath, new byte[0], existsPolicy);
- zoo.putPersistentData(zPath + Constants.ZNAMESPACE_NAME,
namespace.getBytes(UTF_8),
- existsPolicy);
+ zoo.putPersistentData(zPath + "/" + namespaceId, new byte[0],
existsPolicy);
+ zoo.putPersistentData(zPath,
+ NamespaceMapping.writeNamespaceToMap(zoo, zPath, namespaceId,
namespace), existsPolicy);
Review Comment:
@keith-turner I think this method is only ever called during the init code,
which is in a single thread, and in the fate operation for creating a table,
where there is a lock held. So, concurrency is already handled. That said, it
probably couldn't hurt to use mutateExisting here, to make it safe for future
refactorings of Fate.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.clientImpl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Collections.emptySortedMap;
+
+import java.lang.reflect.Type;
+import java.util.Collections;
+import java.util.Map;
+import java.util.SortedMap;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.data.NamespaceId;
+import org.apache.accumulo.core.fate.zookeeper.ZooCache;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.fate.zookeeper.ZooUtil;
+import org.apache.zookeeper.KeeperException;
+
+import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.reflect.TypeToken;
+import com.google.gson.Gson;
+
+public class NamespaceMapping {
+ private static final Gson gson = new Gson();
+ private final ClientContext context;
+ private volatile SortedMap<NamespaceId,String> currentNamespaceMap =
emptySortedMap();
+ private volatile SortedMap<String,NamespaceId> currentNamespaceReverseMap =
emptySortedMap();
+ private volatile long lastMzxid;
+
+ public NamespaceMapping(ClientContext context) {
+ this.context = context;
+ }
+
+ public static void initializeNamespaceMap(ZooReaderWriter zoo, String zPath)
+ throws InterruptedException, KeeperException {
+ Map<String,String> map = Map.of(Namespace.DEFAULT.id().canonical(),
Namespace.DEFAULT.name(),
+ Namespace.ACCUMULO.id().canonical(), Namespace.ACCUMULO.name());
+ zoo.putPersistentData(zPath, serialize(map),
ZooUtil.NodeExistsPolicy.OVERWRITE);
+ }
+
+ public static byte[] writeNamespaceToMap(ZooReaderWriter zoo, String zPath,
+ NamespaceId namespaceId, String namespaceName) throws
InterruptedException, KeeperException {
+ byte[] updatedMap = new byte[0];
+ if (!Namespace.DEFAULT.id().equals(namespaceId)
+ && !Namespace.ACCUMULO.id().equals(namespaceId)) {
+ byte[] data = zoo.getData(zPath);
+ Map<String,String> namespaceMap = deserialize(data);
+ namespaceMap.put(namespaceId.canonical(), namespaceName);
+ updatedMap = serialize(namespaceMap);
+ }
+ return updatedMap;
+ }
+
+ public static byte[] serialize(Map<String,String> map) {
+ var sortedMap =
ImmutableSortedMap.<String,String>naturalOrder().putAll(map).build();
+ String jsonData = gson.toJson(sortedMap);
+ return jsonData.getBytes(UTF_8);
+ }
+
+ public static Map<String,String> deserialize(byte[] data) {
+ if (data == null || data.length == 0) {
+ return Collections.emptyMap();
+ }
+ String jsonData = new String(data, UTF_8);
+ Type type = new TypeToken<Map<String,String>>() {}.getType();
+ return gson.fromJson(jsonData, type);
+ }
+
+ private synchronized void update() {
+ final ZooCache zc = context.getZooCache();
+ final String zPath = context.getZooKeeperRoot() + Constants.ZNAMESPACES;
+ final ZooCache.ZcStat stat = new ZooCache.ZcStat();
+ zc.clear();
+
+ byte[] data = zc.get(zPath, stat);
+ if (stat.getMzxid() > lastMzxid) {
+ if (data == null) {
+ currentNamespaceMap = emptySortedMap();
+ currentNamespaceReverseMap = emptySortedMap();
Review Comment:
I wonder if this should be an error, before it even gets to the deserialize
method?
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java:
##########
@@ -117,6 +120,30 @@ public void upgradeZookeeper(@NonNull ServerContext
context) {
zrw.overwritePersistentData(rootBase, rtm.toJson().getBytes(UTF_8),
stat.getVersion());
log.info("Root metadata in ZooKeeper after upgrade: {}", rtm.toJson());
}
+
+ String zPath = Constants.ZROOT + "/" + context.getInstanceID() +
Constants.ZNAMESPACES;
+ byte[] existingMapData = zrw.getData(zPath);
+ List<String> namespaceIdList = zrw.getChildren(zPath);
+ Map<String,String> namespaceMap = null;
+ if (existingMapData != null) {
+ namespaceMap = NamespaceMapping.deserialize(existingMapData);
+ }
+ if (namespaceMap == null || namespaceMap.isEmpty()) {
+ namespaceMap = new HashMap<>();
+ for (String namespaceId : namespaceIdList) {
+ @SuppressWarnings("deprecation")
+ String namespaceNamePath = zPath + "/" + namespaceId +
Constants.ZNAMESPACE_NAME;
+ namespaceMap.put(namespaceId, new
String(zrw.getData(namespaceNamePath), UTF_8));
+ }
+ byte[] mapping = NamespaceMapping.serialize(namespaceMap);
+ zrw.putPersistentData(zPath, mapping,
ZooUtil.NodeExistsPolicy.OVERWRITE);
+ }
+ for (String namespaceId : namespaceIdList) {
+ @SuppressWarnings("deprecation")
+ String namespaceNamePath = zPath + "/" + namespaceId +
Constants.ZNAMESPACE_NAME;
+ zrw.delete(namespaceNamePath);
Review Comment:
@keith-turner The per-namespace ZK config is stored under each individual
namespaceID. Only the name mapping node is being removed. So, the node is still
in use by other parts of the code. The only item I can think of immediately is
the namespace config, but there is possibly other ZK nodes under each separate
namespace, too, that I just can't remember right now.
--
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]
