keith-turner commented on code in PR #4996:
URL: https://github.com/apache/accumulo/pull/4996#discussion_r1813771801
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/Namespaces.java:
##########
@@ -70,57 +65,31 @@ public static List<String> getTableNames(ClientContext
context, NamespaceId name
return names;
}
- /**
- * Gets all the namespaces from ZK. The first arg (t) the BiConsumer accepts
is the ID and the
- * second (u) is the namespaceName.
- */
- private static void getAllNamespaces(ClientContext context,
- BiConsumer<String,String> biConsumer) {
- final ZooCache zc = context.getZooCache();
- List<String> namespaceIds = zc.getChildren(context.getZooKeeperRoot() +
Constants.ZNAMESPACES);
- for (String id : namespaceIds) {
- byte[] path = zc.get(context.getZooKeeperRoot() + Constants.ZNAMESPACES
+ "/" + id
- + Constants.ZNAMESPACE_NAME);
- if (path != null) {
- biConsumer.accept(id, new String(path, UTF_8));
- }
- }
- }
-
/**
* Return sorted map with key = ID, value = namespaceName
*/
public static SortedMap<NamespaceId,String> getIdToNameMap(ClientContext
context) {
- SortedMap<NamespaceId,String> idMap = new TreeMap<>();
- getAllNamespaces(context, (id, name) -> idMap.put(NamespaceId.of(id),
name));
- return idMap;
+ return context.getNamespaces().getIdToNameMap();
Review Comment:
This method could be refactored away by inlining it.
##########
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:
This code does not seems like it is handling concurrent updates to the new
namespace node correctly. Can use use the mutateExisting function to do this.
This function will attempt to update a zk node and automatically retry if there
was a change to the node between reading the data and writing the updated.
Also not sure why one of the writes to zookeeper is being done.
```suggestion
// Why is the following be added to zookeeper?
zoo.putPersistentData(zPath + "/" + namespaceId, new byte[0],
existsPolicy);
zoo.mutateExisting(zPath, data->{
var namespaces = NamespaceMapping.deserialize(data);
// TODO throw exception if namespace name already exists in map?
namespaces.put(namespaceId.canonical(),namespace);
return NamespaceMapping.serialize(namespaces);
});
```
##########
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();
Review Comment:
Do not want to clear the zoocache for two reasons. One other things use the
cache and this would disrupt those. Second the zoocache should update when
things are updated in zookeeper.
Clearing the zoocache and then reading something from it will always read
from zookeeper, so there is no benefit to using the zoocache.
```suggestion
```
##########
core/src/main/java/org/apache/accumulo/core/Constants.java:
##########
@@ -47,6 +47,7 @@ public class Constants {
public static final String ZTABLE_NAMESPACE = "/namespace";
public static final String ZNAMESPACES = "/namespaces";
+ @Deprecated
public static final String ZNAMESPACE_NAME = "/name";
Review Comment:
Instead of deprecating this code here, it would be better to move the
constant to the Upgrader11to12 class. Then eventually that class will be
deleted when that upgrade code is no longer needed and this constant will be
deleted at the same time.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/Namespaces.java:
##########
@@ -18,15 +18,10 @@
*/
package org.apache.accumulo.core.clientImpl;
-import static java.nio.charset.StandardCharsets.UTF_8;
-
-import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Map.Entry;
import java.util.SortedMap;
-import java.util.TreeMap;
-import java.util.function.BiConsumer;
Review Comment:
This comment is not for this line. Later in this class there is an exists
method. That method could use the new NAmeSpaceMapping object and it may be
more efficient because it would do a map lookup instead of a linear scan of a
list.
##########
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:
The way this code works does not seem quite correct. It is reading data
form zookeeper and passing it into mutate existing. Need to let
mutateExisting do all of the read and writing to zookeeper inorder to correctly
handle concurrent updates. Should probably do something similar to a previous
comment I made about using mutateExisting. May want to move this code that
calls mutateExsting into the new NamespaceMapping class.
##########
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:
If an empty map is serialized then it will be `{}` in json, so would not be
empty string. Not sure of the best way to handle this, but we would not expect
an empty string to stored in zookeeper so should probably not silently ignore
something we do not expect.
##########
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:
I asked this in another comment, why keep these id nodes around in
zookeeper? Wondering if those could be delete also.
```suggestion
zrw.delete(namespaceNamePath);
zrw.delete(zPath + "/" + namespaceId);
```
--
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]
