You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by milleruntime <gi...@git.apache.org> on 2017/07/10 15:38:05 UTC
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
GitHub user milleruntime opened a pull request:
https://github.com/apache/accumulo/pull/279
ACCUMULO-3238 Table.ID Namespace.ID Refactor
* Replaced all occurrences of tableId and namespaceId Strings
that aren't a part of the user facing API with type safe
Table.ID and Namespace.ID objects.
* Table.ID and Namespace.ID both extend the abstract class AbstractId
* Created new methods in Tables and Namespaces for internal
use of these type safe objects.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/milleruntime/accumulo ACCUMULO-3238
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/279.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #279
----
commit 7b6d7cd70f4427475abb1014e74e5a692ee3c470
Author: Mike Miller <mm...@apache.org>
Date: 2017-07-05T21:06:40Z
ACCUMULO-3238 Table.ID Namespace.ID Refactor
* Replaced all occurrences of tableId and namespaceId Strings
that aren't a part of the user facing API with type safe
Table.ID and Namespace.ID objects.
* Created new methods in Tables and Namespaces for internal
use of these type safe objects.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126506938
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -72,10 +78,10 @@ public String invalidMessage(String namespace) {
}
};
- public static final String DEFAULT_NAMESPACE_ID = "+default";
- public static final String DEFAULT_NAMESPACE = "";
- public static final String ACCUMULO_NAMESPACE_ID = "+accumulo";
- public static final String ACCUMULO_NAMESPACE = "accumulo";
+ public static final Namespace.ID DEFAULT_NAMESPACE_ID = Namespace.ID.DEFAULT;
+ public static final String DEFAULT_NAMESPACE = Namespace.DEFAULT;
+ public static final Namespace.ID ACCUMULO_NAMESPACE_ID = Namespace.ID.ACCUMULO;
+ public static final String ACCUMULO_NAMESPACE = Namespace.ACCUMULO;
--- End diff --
Could probably remove these in future, replacing them with what they point to.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126551778
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
--- End diff --
I am not sure exactly why but without it findbugs will flag a bunch of classes in master with this bug:
[INFO] Class org.apache.accumulo.master.tableOps.BulkImport defines non-transient non-serializable instance field tableId [org.apache.accumulo.master.tableOps.BulkImport] In BulkImport.java SE_BAD_FIELD
It seems like everything in tableOps with a tableId requires it (I guess because of FATE operations?)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r127058627
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespace.java ---
@@ -0,0 +1,42 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+
+public class Namespace {
--- End diff --
> I'm reticent to include structure on the promise of "code getting finished"
With Accumulo being such a complex and mature project, I think incremental code changes should be encouraged. It is very difficult to make significant code changes all at once.
> this just seems like a layer of indirection that causes more typing
If useful code can be commited, while at the same time creating opportunity to assist in future significant development, I think it is definitely worth typing the extra "."
Should I add comments to the javadoc explaining how the class is also a placeholder for future work?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126722326
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
+ }
+
+ public ID(final String canonical) {
+ super(canonical);
+ }
+
+ public ID(Text textID) {
--- End diff --
Could avoid use of Hadoop's `Text` object and just call `new ID(textObj.toString())` any where in code where it is used.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126742423
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
public static String getNamespaceName(Instance instance, String namespaceId) throws NamespaceNotFoundException {
String namespaceName = getIdToNameMap(instance).get(namespaceId);
if (namespaceName == null)
throw new NamespaceNotFoundException(namespaceId, null, "getNamespaceName() failed to find namespace");
return namespaceName;
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getNameMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getNameToIdMap(Instance instance) {
return getMap(instance, true);
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getIdMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getIdToNameMap(Instance instance) {
return getMap(instance, false);
}
- public static List<String> getTableIds(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<Table.ID> getTableIds(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
- List<String> names = new LinkedList<>();
- for (Entry<String,String> nameToId : Tables.getNameToIdMap(instance).entrySet())
+ List<Table.ID> tableIds = new LinkedList<>();
+ for (Entry<String,Table.ID> nameToId : Tables.getNameMap(instance).entrySet())
if (namespace.equals(Tables.qualify(nameToId.getKey()).getFirst()))
- names.add(nameToId.getValue());
- return names;
+ tableIds.add(nameToId.getValue());
+ return tableIds;
}
- public static List<String> getTableNames(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<String> getTableNames(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
List<String> names = new LinkedList<>();
- for (String name : Tables.getNameToIdMap(instance).keySet())
+ for (String name : Tables.getNameMap(instance).keySet())
if (namespace.equals(Tables.qualify(name).getFirst()))
names.add(name);
return names;
}
+ /**
+ * Populate map passed in as the BiConsumer. key = ID, value = namespaceName
+ */
+ private static void populateMap(Instance instance, BiConsumer<String,String> biConsumer) {
+ final ZooCache zc = getZooCache(instance);
+ List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
+ for (String id : namespaceIds) {
+ byte[] path = zc.get(ZooUtil.getRoot(instance) + 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<Namespace.ID,String> getIdMap(Instance instance) {
+ SortedMap<Namespace.ID,String> idMap = new TreeMap<>();
+ populateMap(instance, (id, name) -> idMap.put(new Namespace.ID(id), name));
+ return idMap;
+ }
+
+ /**
+ * Return sorted map with key = namespaceName, value = ID
+ */
+ public static SortedMap<String,Namespace.ID> getNameMap(Instance instance) {
+ SortedMap<String,Namespace.ID> nameMap = new TreeMap<>();
+ populateMap(instance, (id, name) -> nameMap.put(name, new Namespace.ID(id)));
+ return nameMap;
+ }
+
+ /**
+ * Look for namespace ID in ZK. Throw NamespaceNotFoundException if not found.
+ */
+ public static Namespace.ID getNamespaceId(Instance instance, String namespaceName) throws NamespaceNotFoundException {
+ final ArrayList<Namespace.ID> singleId = new ArrayList<>(1);
+ populateMap(instance, (id, name) -> {
+ if (name.equals(namespaceName))
+ singleId.add(new Namespace.ID(id));
+ });
+ if (singleId.isEmpty())
+ throw new NamespaceNotFoundException(null, namespaceName, "getNamespaceId() failed to find namespace");
+ return singleId.get(0);
+ }
+
+ /**
+ * Look for namespace ID in ZK. Fail quietly by logging and returning null.
+ */
+ public static Namespace.ID lookupNamespaceId(Instance instance, String namespaceName) {
+ Namespace.ID id = null;
+ try {
+ id = getNamespaceId(instance, namespaceName);
+ } catch (NamespaceNotFoundException e) {
+ if (log.isDebugEnabled())
+ log.debug("Failed to find namespace ID from name: " + namespaceName, e);
+ }
+ return id;
+ }
+
+ /**
+ * Return true if namespace name exists
+ */
+ public static boolean namespaceNameExists(Instance instance, String namespaceName) {
+ return lookupNamespaceId(instance, namespaceName) != null;
+ }
+
+ /**
+ * Look for namespace name in ZK. Throw NamespaceNotFoundException if not found.
+ */
+ public static String getNamespaceName(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
+ final ArrayList<String> singleName = new ArrayList<>(1);
+ populateMap(instance, (id, name) -> {
+ if (id.equals(namespaceId.canonicalID()))
+ singleName.add(name);
+ });
+ if (singleName.isEmpty())
+ throw new NamespaceNotFoundException(namespaceId.canonicalID(), null, "getNamespaceName() failed to find namespace");
+ return singleName.get(0);
+ }
+
+ /**
+ * Look for namespace name in ZK. Fail quietly by logging and returning null.
+ */
+ public static String lookupNamespaceName(Instance instance, Namespace.ID namespaceId) {
+ String name = null;
+ try {
+ getNamespaceName(instance, namespaceId);
--- End diff --
Why suppress this exception? It seems like something that should propagate to the caller.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126531488
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -72,10 +78,10 @@ public String invalidMessage(String namespace) {
}
};
- public static final String DEFAULT_NAMESPACE_ID = "+default";
- public static final String DEFAULT_NAMESPACE = "";
- public static final String ACCUMULO_NAMESPACE_ID = "+accumulo";
- public static final String ACCUMULO_NAMESPACE = "accumulo";
+ public static final Namespace.ID DEFAULT_NAMESPACE_ID = Namespace.ID.DEFAULT;
+ public static final String DEFAULT_NAMESPACE = Namespace.DEFAULT;
+ public static final Namespace.ID ACCUMULO_NAMESPACE_ID = Namespace.ID.ACCUMULO;
+ public static final String ACCUMULO_NAMESPACE = Namespace.ACCUMULO;
--- End diff --
Agreed. This was an attempt to minimize number of changes in one PR. Assuming there aren't any showstoppers with these changes, I will create follow on ticket.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126758739
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespace.java ---
@@ -0,0 +1,42 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+
+public class Namespace {
--- End diff --
Thanks for the context, you two.
I'm reticent to include structure on the promise of "code getting finished" as this is all internal anyways. Without the new public API codebase being anywhere close to inclusion, this just seems like a layer of indirection that causes more typing. At the same time, I can't think of a reason that it's harmful -- it doesn't make sense to me to introduce these classes without any immediate benefit/gain.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126742538
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
--- End diff --
Rather than keep the old ones around, I would just change the implementation of the API methods to one which does something like:
```java
return Tables.getNameToIdMap().entrySet().stream()
.collect(Collectors.toMap(Map.Entry::getKey,
e -> e.canonicalId(),
(v1,v2) ->{ throw new RuntimeException(String.format("Duplicate key for values %s and %s", v1, v2));},
TreeMap::new)));
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126759594
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
public static String getNamespaceName(Instance instance, String namespaceId) throws NamespaceNotFoundException {
String namespaceName = getIdToNameMap(instance).get(namespaceId);
if (namespaceName == null)
throw new NamespaceNotFoundException(namespaceId, null, "getNamespaceName() failed to find namespace");
return namespaceName;
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getNameMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getNameToIdMap(Instance instance) {
return getMap(instance, true);
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getIdMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getIdToNameMap(Instance instance) {
return getMap(instance, false);
}
- public static List<String> getTableIds(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<Table.ID> getTableIds(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
- List<String> names = new LinkedList<>();
- for (Entry<String,String> nameToId : Tables.getNameToIdMap(instance).entrySet())
+ List<Table.ID> tableIds = new LinkedList<>();
+ for (Entry<String,Table.ID> nameToId : Tables.getNameMap(instance).entrySet())
if (namespace.equals(Tables.qualify(nameToId.getKey()).getFirst()))
- names.add(nameToId.getValue());
- return names;
+ tableIds.add(nameToId.getValue());
+ return tableIds;
}
- public static List<String> getTableNames(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<String> getTableNames(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
List<String> names = new LinkedList<>();
- for (String name : Tables.getNameToIdMap(instance).keySet())
+ for (String name : Tables.getNameMap(instance).keySet())
if (namespace.equals(Tables.qualify(name).getFirst()))
names.add(name);
return names;
}
+ /**
+ * Populate map passed in as the BiConsumer. key = ID, value = namespaceName
+ */
+ private static void populateMap(Instance instance, BiConsumer<String,String> biConsumer) {
+ final ZooCache zc = getZooCache(instance);
+ List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
+ for (String id : namespaceIds) {
+ byte[] path = zc.get(ZooUtil.getRoot(instance) + 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<Namespace.ID,String> getIdMap(Instance instance) {
+ SortedMap<Namespace.ID,String> idMap = new TreeMap<>();
+ populateMap(instance, (id, name) -> idMap.put(new Namespace.ID(id), name));
+ return idMap;
+ }
+
+ /**
+ * Return sorted map with key = namespaceName, value = ID
+ */
+ public static SortedMap<String,Namespace.ID> getNameMap(Instance instance) {
+ SortedMap<String,Namespace.ID> nameMap = new TreeMap<>();
+ populateMap(instance, (id, name) -> nameMap.put(name, new Namespace.ID(id)));
+ return nameMap;
+ }
+
+ /**
+ * Look for namespace ID in ZK. Throw NamespaceNotFoundException if not found.
+ */
+ public static Namespace.ID getNamespaceId(Instance instance, String namespaceName) throws NamespaceNotFoundException {
+ final ArrayList<Namespace.ID> singleId = new ArrayList<>(1);
+ populateMap(instance, (id, name) -> {
+ if (name.equals(namespaceName))
+ singleId.add(new Namespace.ID(id));
+ });
+ if (singleId.isEmpty())
+ throw new NamespaceNotFoundException(null, namespaceName, "getNamespaceId() failed to find namespace");
+ return singleId.get(0);
+ }
+
+ /**
+ * Look for namespace ID in ZK. Fail quietly by logging and returning null.
+ */
+ public static Namespace.ID lookupNamespaceId(Instance instance, String namespaceName) {
+ Namespace.ID id = null;
+ try {
+ id = getNamespaceId(instance, namespaceName);
+ } catch (NamespaceNotFoundException e) {
+ if (log.isDebugEnabled())
+ log.debug("Failed to find namespace ID from name: " + namespaceName, e);
+ }
+ return id;
+ }
+
+ /**
+ * Return true if namespace name exists
+ */
+ public static boolean namespaceNameExists(Instance instance, String namespaceName) {
+ return lookupNamespaceId(instance, namespaceName) != null;
+ }
+
+ /**
+ * Look for namespace name in ZK. Throw NamespaceNotFoundException if not found.
+ */
+ public static String getNamespaceName(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
+ final ArrayList<String> singleName = new ArrayList<>(1);
+ populateMap(instance, (id, name) -> {
+ if (id.equals(namespaceId.canonicalID()))
+ singleName.add(name);
+ });
+ if (singleName.isEmpty())
+ throw new NamespaceNotFoundException(namespaceId.canonicalID(), null, "getNamespaceName() failed to find namespace");
+ return singleName.get(0);
+ }
+
+ /**
+ * Look for namespace name in ZK. Fail quietly by logging and returning null.
+ */
+ public static String lookupNamespaceName(Instance instance, Namespace.ID namespaceId) {
+ String name = null;
+ try {
+ getNamespaceName(instance, namespaceId);
--- End diff --
I don't think this is something @milleruntime added. It probably already existed. I think this kind of thing plagues the whole of `Tables` and `Namespaces` util classes. These were intended for very specific internal use cases, with very specific handling behaviors, and in some cases, warnings were suppressed from common code because the caller had its own handling (possibly with better messages).
Over time, these util methods got used for things they were not originally intended to be used for. It's possible some of the callers depend on the null return value, and others would benefit from the exception being thrown. Without actually tracing all the callers, it's anyone's guess as to how many of these util classes are being used with incorrect assumptions about exception handling. I seem to recall at least a few bugs being fixed because of this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r127780058
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+ }
+
+ @Override
+ public int hashCode() {
+ if (hashCode == null) {
+ hashCode = Objects.hash(canonicalID());
+ }
+ return hashCode;
+ }
+
+ /**
+ * Returns a string of the canonical ID
+ */
+ @Override
+ public String toString() {
+ return canonical;
+ }
+
+ /**
+ * Return a UTF_8 byte[] of the canonical ID.
+ */
+ public final byte[] getUtf8Bytes() {
+ return canonical.getBytes(UTF_8);
--- End diff --
I like `getUtf8()`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126739661
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
--- End diff --
Would be nice to do a reference check (`if (this == obj) return true;`)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126505403
--- Diff: server/master/src/main/java/org/apache/accumulo/master/tableOps/WriteExportFiles.java ---
@@ -144,16 +145,15 @@ public static void exportTable(VolumeManager fs, AccumuloServerContext context,
BufferedOutputStream bufOut = new BufferedOutputStream(zipOut);
DataOutputStream dataOut = new DataOutputStream(bufOut);
- try {
+ try (OutputStreamWriter osw = new OutputStreamWriter(dataOut, UTF_8)) {
--- End diff --
This got dinged by Findbugs for not closing the stream so I had put the OutputStreamWriter in the try with resources.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126754662
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
+ }
+
+ public ID(final String canonical) {
+ super(canonical);
+ }
+
+ public ID(Text textID) {
--- End diff --
Good idea. I created this constructor for ease of use with Thrift RPC calls that use Text but I think it would be worth it to remove the Hadoop dependency.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126509016
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
+ }
+
+ public ID(final String canonical) {
+ super(canonical);
+ }
+
+ public ID(Text textID) {
+ super(textID.toString());
+ }
+ }
+
--- End diff --
Noticed you had constants for namespace "ACCUMULO" and "DEFAULT" names, but no constants here for built-in table names. Not strictly necessary, but would be nice to be consistent.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126746910
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
--- End diff --
The class is already an identifier (ID). This string is the canonical form of that ID for serialization, etc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r127068016
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
+ }
+
+ public ID(final String canonical) {
+ super(canonical);
+ }
+
+ public ID(Text textID) {
--- End diff --
This turned out to be fairly easy. Fixed in ef30550
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126729802
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
--- End diff --
From https://docs.oracle.com/javase/tutorial/java/javaOO/nested.html it looks serialization of nested classes is strongly discouraged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126746264
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
--- End diff --
Won't this loop through all tables/namespaces twice?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126755404
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+ }
+
+ @Override
+ public int hashCode() {
+ if (hashCode == null) {
+ hashCode = Objects.hash(canonicalID());
+ }
+ return hashCode;
+ }
+
+ /**
+ * Returns a string of the canonical ID
+ */
+ @Override
+ public String toString() {
+ return canonical;
+ }
+
+ /**
+ * Return a UTF_8 byte[] of the canonical ID.
+ */
+ public final byte[] getUtf8Bytes() {
+ return canonical.getBytes(UTF_8);
--- End diff --
One reason not to use `getBytes` is that because of the encoding issues that plague current methods with that name, the name `getBytes` is almost always a red flag that encoding has been overlooked. Concretely selecting a different name communicates to the developer reading calls to it that they don't need to stop and double-check the docs and/or implementation to ensure encoding is handled correctly.
Another reason to avoid the common name. Searching for references in IDEs like Eclipse will also turn up thousands, if not millions, of false positive matches from `String.getBytes()` and others.
I'd almost prefer `getUtf8()` over `getBytes`.
I believe this method only exists as a convenience method for constructing metadata rows. It can probably be removed from this class, and be pushed down into the metadata code where it's needed. Keep the serialization with the storage, is probably preferred in this case, than keeping the serialization with the object.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126758011
--- Diff: server/base/src/main/java/org/apache/accumulo/server/master/balancer/GroupBalancer.java ---
@@ -70,15 +70,15 @@
public abstract class GroupBalancer extends TabletBalancer {
- private final String tableId;
+ private final Table.ID tableId;
private long lastRun = 0;
/**
* @return A function that groups tablets into named groups.
*/
protected abstract Function<KeyExtent,String> getPartitioner();
- public GroupBalancer(String tableId) {
+ public GroupBalancer(Table.ID tableId) {
--- End diff --
Sounds good. Its not worth breaking user code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126763252
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
--- End diff --
In that example, it could just be initialized to null. The variable is only being created outside the try so that it exists for the exception. But, "Table null not found" is probably slightly better than "Table not found" in an exception message, for debugging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126742200
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
public static String getNamespaceName(Instance instance, String namespaceId) throws NamespaceNotFoundException {
String namespaceName = getIdToNameMap(instance).get(namespaceId);
if (namespaceName == null)
throw new NamespaceNotFoundException(namespaceId, null, "getNamespaceName() failed to find namespace");
return namespaceName;
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getNameMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getNameToIdMap(Instance instance) {
return getMap(instance, true);
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getIdMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getIdToNameMap(Instance instance) {
return getMap(instance, false);
}
- public static List<String> getTableIds(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<Table.ID> getTableIds(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
- List<String> names = new LinkedList<>();
- for (Entry<String,String> nameToId : Tables.getNameToIdMap(instance).entrySet())
+ List<Table.ID> tableIds = new LinkedList<>();
+ for (Entry<String,Table.ID> nameToId : Tables.getNameMap(instance).entrySet())
if (namespace.equals(Tables.qualify(nameToId.getKey()).getFirst()))
- names.add(nameToId.getValue());
- return names;
+ tableIds.add(nameToId.getValue());
+ return tableIds;
}
- public static List<String> getTableNames(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<String> getTableNames(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
List<String> names = new LinkedList<>();
- for (String name : Tables.getNameToIdMap(instance).keySet())
+ for (String name : Tables.getNameMap(instance).keySet())
if (namespace.equals(Tables.qualify(name).getFirst()))
names.add(name);
return names;
}
+ /**
+ * Populate map passed in as the BiConsumer. key = ID, value = namespaceName
+ */
+ private static void populateMap(Instance instance, BiConsumer<String,String> biConsumer) {
+ final ZooCache zc = getZooCache(instance);
+ List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
+ for (String id : namespaceIds) {
+ byte[] path = zc.get(ZooUtil.getRoot(instance) + 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<Namespace.ID,String> getIdMap(Instance instance) {
+ SortedMap<Namespace.ID,String> idMap = new TreeMap<>();
+ populateMap(instance, (id, name) -> idMap.put(new Namespace.ID(id), name));
+ return idMap;
+ }
+
+ /**
+ * Return sorted map with key = namespaceName, value = ID
+ */
+ public static SortedMap<String,Namespace.ID> getNameMap(Instance instance) {
+ SortedMap<String,Namespace.ID> nameMap = new TreeMap<>();
+ populateMap(instance, (id, name) -> nameMap.put(name, new Namespace.ID(id)));
+ return nameMap;
+ }
+
+ /**
+ * Look for namespace ID in ZK. Throw NamespaceNotFoundException if not found.
+ */
+ public static Namespace.ID getNamespaceId(Instance instance, String namespaceName) throws NamespaceNotFoundException {
+ final ArrayList<Namespace.ID> singleId = new ArrayList<>(1);
+ populateMap(instance, (id, name) -> {
+ if (name.equals(namespaceName))
+ singleId.add(new Namespace.ID(id));
+ });
+ if (singleId.isEmpty())
+ throw new NamespaceNotFoundException(null, namespaceName, "getNamespaceId() failed to find namespace");
+ return singleId.get(0);
+ }
+
+ /**
+ * Look for namespace ID in ZK. Fail quietly by logging and returning null.
+ */
+ public static Namespace.ID lookupNamespaceId(Instance instance, String namespaceName) {
+ Namespace.ID id = null;
+ try {
+ id = getNamespaceId(instance, namespaceName);
+ } catch (NamespaceNotFoundException e) {
+ if (log.isDebugEnabled())
+ log.debug("Failed to find namespace ID from name: " + namespaceName, e);
+ }
+ return id;
+ }
+
+ /**
+ * Return true if namespace name exists
+ */
+ public static boolean namespaceNameExists(Instance instance, String namespaceName) {
+ return lookupNamespaceId(instance, namespaceName) != null;
+ }
+
+ /**
+ * Look for namespace name in ZK. Throw NamespaceNotFoundException if not found.
+ */
+ public static String getNamespaceName(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
+ final ArrayList<String> singleName = new ArrayList<>(1);
+ populateMap(instance, (id, name) -> {
--- End diff --
Why iterate over every namespace to just find a single one? Creating a new List also seems unnecessary to just find the namespace from an ID..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126762674
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
--- End diff --
OK good to know.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126760182
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
--- End diff --
It does seem odd but I created this to replace initializations of empty string. Like here: https://github.com/milleruntime/accumulo/blob/master/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java#L784
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126511167
--- Diff: server/base/src/main/java/org/apache/accumulo/server/master/balancer/GroupBalancer.java ---
@@ -70,15 +70,15 @@
public abstract class GroupBalancer extends TabletBalancer {
- private final String tableId;
+ private final Table.ID tableId;
private long lastRun = 0;
/**
* @return A function that groups tablets into named groups.
*/
protected abstract Function<KeyExtent,String> getPartitioner();
- public GroupBalancer(String tableId) {
+ public GroupBalancer(Table.ID tableId) {
--- End diff --
This isn't strictly public API, but the balancer is one of those internal "points of code injection", for users to change Accumulo's default behavior. If API change can be avoided, that is preferred here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126753848
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
+ }
+
+ public ID(final String canonical) {
+ super(canonical);
+ }
+
+ public ID(Text textID) {
+ super(textID.toString());
+ }
+ }
+
--- End diff --
I didn't add constants for built-in table names because MetadataTable.NAME and RootTable.NAME already exist and are widely used. I could create a follow on to move the constants from these classes into Table.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126734863
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
--- End diff --
I kept the deprecated methods around because of the 2 API methods NamespaceOperations Map<String,String> namespaceIdMap() and TableOperations Map<String,String> tableIdMap(). If I remove the deprecated methods then I either have to make populateMap(Instance, BiConsumer<String,String>) public or create a similar method that returns Map<String,String>. I figured at least with the methods deprecated, we can communicate the preferred methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126757222
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespace.java ---
@@ -0,0 +1,42 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+
+public class Namespace {
--- End diff --
> Is there some kind of longer-term goal with what the Namespace and Table classes will become?
Yes this name came from ACCUMULO-3238 and the (unfinished) work @ctubbsii did for ACCUMULO-2589. Basically, creating the opportunity for more useful Table and Namespace objects, while improving type safety in the meantime. Plus there is already org.apache.accumulo.core.data.TabletId
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126765808
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespace.java ---
@@ -0,0 +1,42 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+
+public class Namespace {
--- End diff --
The immediate benefit was as you mentioned... naming. The fact that it's also a foundation for future work is great, but there's at least some benefit now (even if naming is a trivial benefit). In any case, the wrapper is also used for some convenience constants, and isn't hurting anything. It's still a single file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126737922
--- Diff: core/src/main/java/org/apache/accumulo/core/client/TableOfflineException.java ---
@@ -26,7 +27,7 @@ private static String getTableName(Instance instance, String tableId) {
if (tableId == null)
return " <unknown table> ";
try {
- String tableName = Tables.getTableName(instance, tableId);
+ String tableName = Tables.getTableName(instance, new Table.ID(tableId));
--- End diff --
HBase does this with their `TableName` class to avoid tons of objects hanging around needing GC. The API is pretty nice as an end-user:
```java
Connection conn = ...;
Table t = conn.getTable(TableName.valueOf("josh_table1"));
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126501184
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
--- End diff --
Do we need this to be serializable right now? Where are these serialized? Will that break compatibility with previously serialized objects?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126739863
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+ }
+
+ @Override
+ public int hashCode() {
+ if (hashCode == null) {
+ hashCode = Objects.hash(canonicalID());
+ }
+ return hashCode;
+ }
+
+ /**
+ * Returns a string of the canonical ID
+ */
+ @Override
+ public String toString() {
+ return canonical;
+ }
+
+ /**
+ * Return a UTF_8 byte[] of the canonical ID.
+ */
+ public final byte[] getUtf8Bytes() {
+ return canonical.getBytes(UTF_8);
--- End diff --
Why this distinction? Is there a reason that we'd ever _not_ want the canonical name to be UTF-8?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r127067656
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
--- End diff --
I was able to work around the need for empty() everywhere except KeyExtent, where I just created a local EMPTY_ID to prevent any NPEs. ef30550
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126741465
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
public static String getNamespaceName(Instance instance, String namespaceId) throws NamespaceNotFoundException {
String namespaceName = getIdToNameMap(instance).get(namespaceId);
if (namespaceName == null)
throw new NamespaceNotFoundException(namespaceId, null, "getNamespaceName() failed to find namespace");
return namespaceName;
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getNameMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getNameToIdMap(Instance instance) {
return getMap(instance, true);
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getIdMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getIdToNameMap(Instance instance) {
return getMap(instance, false);
}
- public static List<String> getTableIds(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<Table.ID> getTableIds(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
- List<String> names = new LinkedList<>();
- for (Entry<String,String> nameToId : Tables.getNameToIdMap(instance).entrySet())
+ List<Table.ID> tableIds = new LinkedList<>();
+ for (Entry<String,Table.ID> nameToId : Tables.getNameMap(instance).entrySet())
if (namespace.equals(Tables.qualify(nameToId.getKey()).getFirst()))
- names.add(nameToId.getValue());
- return names;
+ tableIds.add(nameToId.getValue());
+ return tableIds;
--- End diff --
Depending on how the caller uses the return value, it could also be lazily constructed instead of instantiating a new Collection each time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126745260
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
--- End diff --
I believe this only applies to non static inner classes. https://www.securecoding.cert.org/confluence/display/java/SER05-J.+Do+not+serialize+instances+of+inner+classes
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126748404
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+ }
+
+ @Override
+ public int hashCode() {
+ if (hashCode == null) {
+ hashCode = Objects.hash(canonicalID());
+ }
+ return hashCode;
+ }
+
+ /**
+ * Returns a string of the canonical ID
+ */
+ @Override
+ public String toString() {
+ return canonical;
+ }
+
+ /**
+ * Return a UTF_8 byte[] of the canonical ID.
+ */
+ public final byte[] getUtf8Bytes() {
+ return canonical.getBytes(UTF_8);
--- End diff --
Not that I know of. I just gave the method a more descriptive name instead of just "getBytes".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126589554
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
--- End diff --
Stuff in tableOps are FATE operations, which are serialized and stored in ZK. So, this object must also be Serializable, so it can be included in the FATE serialization. That's unfortunate. It's probably okay, as long as we check for outstanding FATE operations from the previous shutdown, before we modify *ANYTHING* during the upgrade process, because we won't be able to deserialize FATEs from before the upgrade.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126508570
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
public static String getNamespaceName(Instance instance, String namespaceId) throws NamespaceNotFoundException {
String namespaceName = getIdToNameMap(instance).get(namespaceId);
if (namespaceName == null)
throw new NamespaceNotFoundException(namespaceId, null, "getNamespaceName() failed to find namespace");
return namespaceName;
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getNameMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getNameToIdMap(Instance instance) {
return getMap(instance, true);
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getIdMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getIdToNameMap(Instance instance) {
return getMap(instance, false);
}
- public static List<String> getTableIds(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<Table.ID> getTableIds(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
- List<String> names = new LinkedList<>();
- for (Entry<String,String> nameToId : Tables.getNameToIdMap(instance).entrySet())
+ List<Table.ID> tableIds = new LinkedList<>();
+ for (Entry<String,Table.ID> nameToId : Tables.getNameMap(instance).entrySet())
if (namespace.equals(Tables.qualify(nameToId.getKey()).getFirst()))
- names.add(nameToId.getValue());
- return names;
+ tableIds.add(nameToId.getValue());
+ return tableIds;
--- End diff --
I bet this could be turned into a clever one-liner with Java 8 lambdas:
```java
String namespace = getNamespaceName(instance, namespaceId);
return Tables.getNameMap(instance).entrySet().stream().filter(e -> namespace.equals(Tables.qualify(e.getKey()).getFirst())).collect(Collectors.toList());
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126726827
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
--- End diff --
This is the same as `toString()` so it could be removed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on the issue:
https://github.com/apache/accumulo/pull/279
I believe I have addressed all concerns. @joshelser if you aren't opposed to the Table and Namespace wrapper classes, I will merge this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126757645
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+ }
+
+ @Override
+ public int hashCode() {
+ if (hashCode == null) {
+ hashCode = Objects.hash(canonicalID());
+ }
+ return hashCode;
+ }
+
+ /**
+ * Returns a string of the canonical ID
+ */
+ @Override
+ public String toString() {
+ return canonical;
+ }
+
+ /**
+ * Return a UTF_8 byte[] of the canonical ID.
+ */
+ public final byte[] getUtf8Bytes() {
+ return canonical.getBytes(UTF_8);
--- End diff --
> because of the encoding issues that plague current methods with that name
That's why Javadoc says the bytes are UTF-8 and not the Java default encoding.
> Searching for references in IDEs like Eclipse will also turn up thousands, if not millions, of false positive matches from String.getBytes() and others.
We should avoid method names in Accumulo based on the frequency of method names used in other projects? Sounds like a rabbit hole I don't want to go down.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126761166
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+ }
+
+ @Override
+ public int hashCode() {
+ if (hashCode == null) {
+ hashCode = Objects.hash(canonicalID());
+ }
+ return hashCode;
+ }
+
+ /**
+ * Returns a string of the canonical ID
+ */
+ @Override
+ public String toString() {
+ return canonical;
+ }
+
+ /**
+ * Return a UTF_8 byte[] of the canonical ID.
+ */
+ public final byte[] getUtf8Bytes() {
+ return canonical.getBytes(UTF_8);
--- End diff --
> We should avoid method names in Accumulo based on the frequency of method names used in other projects? Sounds like a rabbit hole I don't want to go down.
In principle, I agree. But, in practice, I *really* hate not being able to easily track down references to methods, especially when a more descriptive name avoids the issue entirely and has the added benefit of... being more descriptive.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126764150
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
--- End diff --
Could add a comment then to make it clear. I didn't know what was expected given that variable name.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126746697
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
--- End diff --
No. This is bad practice. `toString()` is for human-readable printable forms. It should not be used as a substitute for serialization, normalization, uniqueness, or other computer-parseable forms.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126772637
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
--- End diff --
I agree. There are a few places where a NPE could be thrown. https://github.com/milleruntime/accumulo/blob/ACCUMULO-3238/core/src/main/java/org/apache/accumulo/core/client/mapred/AbstractInputFormat.java#L659
In TableDeletedException(tableID). Although its for mock instance
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126726174
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
--- End diff --
Could change variable name to `identifier`. The name `canonical` is confusing to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126503288
--- Diff: core/src/main/java/org/apache/accumulo/core/client/TableOfflineException.java ---
@@ -26,7 +27,7 @@ private static String getTableName(Instance instance, String tableId) {
if (tableId == null)
return " <unknown table> ";
try {
- String tableName = Tables.getTableName(instance, tableId);
+ String tableName = Tables.getTableName(instance, new Table.ID(tableId));
--- End diff --
Could maybe avoid duplicates by making constructor private and doing `Table.ID.of(tableId)`, which draws from an internal `WeakReference` map. I think we do this in one place with table names. It's not necessary on a first pass, but maybe something to think about if this object creation starts killing performance.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126741153
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespace.java ---
@@ -0,0 +1,42 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+
+public class Namespace {
--- End diff --
The outer-class of `Namespace` (as with `Table`) seems to exist solely for the purpose of the naming.
Is there some kind of longer-term goal with what the `Namespace` and `Table` classes will become? If not, I'd suggest just collapsing this hierarchy to be `TableId` and `NamespaceId`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126509804
--- Diff: core/src/main/java/org/apache/accumulo/core/data/impl/TabletIdImpl.java ---
@@ -36,7 +36,7 @@ public int compareTo(TabletId o) {
@Override
public Text getTableId() {
- return new Text(ke.getTableId());
+ return new Text(ke.getTableId().getUtf8Bytes());
--- End diff --
Could also rely on the string constructor for Text here. It should use UTF8 bytes internally.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r127793774
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+ }
+
+ @Override
+ public int hashCode() {
+ if (hashCode == null) {
+ hashCode = Objects.hash(canonicalID());
+ }
+ return hashCode;
+ }
+
+ /**
+ * Returns a string of the canonical ID
+ */
+ @Override
+ public String toString() {
+ return canonical;
+ }
+
+ /**
+ * Return a UTF_8 byte[] of the canonical ID.
+ */
+ public final byte[] getUtf8Bytes() {
+ return canonical.getBytes(UTF_8);
--- End diff --
Ok I like that. You don't need "bytes" in the name since that will be forced by the return type
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r127045543
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
--- End diff --
I read the tutorial again and you are right.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r127068208
--- Diff: server/base/src/main/java/org/apache/accumulo/server/master/balancer/GroupBalancer.java ---
@@ -70,15 +70,15 @@
public abstract class GroupBalancer extends TabletBalancer {
- private final String tableId;
+ private final Table.ID tableId;
private long lastRun = 0;
/**
* @return A function that groups tablets into named groups.
*/
protected abstract Function<KeyExtent,String> getPartitioner();
- public GroupBalancer(String tableId) {
+ public GroupBalancer(Table.ID tableId) {
--- End diff --
Backed out changes from GroupBalancer and RegexGroupBalancer in ef30550
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126755633
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
--- End diff --
Is this even used for anything? What does it mean to have an empty Table ID?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r127239847
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
public static String getNamespaceName(Instance instance, String namespaceId) throws NamespaceNotFoundException {
String namespaceName = getIdToNameMap(instance).get(namespaceId);
if (namespaceName == null)
throw new NamespaceNotFoundException(namespaceId, null, "getNamespaceName() failed to find namespace");
return namespaceName;
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getNameMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getNameToIdMap(Instance instance) {
return getMap(instance, true);
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getIdMap(Instance)}
+ */
+ @Deprecated
public static SortedMap<String,String> getIdToNameMap(Instance instance) {
return getMap(instance, false);
}
- public static List<String> getTableIds(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<Table.ID> getTableIds(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
- List<String> names = new LinkedList<>();
- for (Entry<String,String> nameToId : Tables.getNameToIdMap(instance).entrySet())
+ List<Table.ID> tableIds = new LinkedList<>();
+ for (Entry<String,Table.ID> nameToId : Tables.getNameMap(instance).entrySet())
if (namespace.equals(Tables.qualify(nameToId.getKey()).getFirst()))
- names.add(nameToId.getValue());
- return names;
+ tableIds.add(nameToId.getValue());
+ return tableIds;
}
- public static List<String> getTableNames(Instance instance, String namespaceId) throws NamespaceNotFoundException {
+ public static List<String> getTableNames(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
String namespace = getNamespaceName(instance, namespaceId);
List<String> names = new LinkedList<>();
- for (String name : Tables.getNameToIdMap(instance).keySet())
+ for (String name : Tables.getNameMap(instance).keySet())
if (namespace.equals(Tables.qualify(name).getFirst()))
names.add(name);
return names;
}
+ /**
+ * Populate map passed in as the BiConsumer. key = ID, value = namespaceName
+ */
+ private static void populateMap(Instance instance, BiConsumer<String,String> biConsumer) {
+ final ZooCache zc = getZooCache(instance);
+ List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
+ for (String id : namespaceIds) {
+ byte[] path = zc.get(ZooUtil.getRoot(instance) + 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<Namespace.ID,String> getIdMap(Instance instance) {
+ SortedMap<Namespace.ID,String> idMap = new TreeMap<>();
+ populateMap(instance, (id, name) -> idMap.put(new Namespace.ID(id), name));
+ return idMap;
+ }
+
+ /**
+ * Return sorted map with key = namespaceName, value = ID
+ */
+ public static SortedMap<String,Namespace.ID> getNameMap(Instance instance) {
+ SortedMap<String,Namespace.ID> nameMap = new TreeMap<>();
+ populateMap(instance, (id, name) -> nameMap.put(name, new Namespace.ID(id)));
+ return nameMap;
+ }
+
+ /**
+ * Look for namespace ID in ZK. Throw NamespaceNotFoundException if not found.
+ */
+ public static Namespace.ID getNamespaceId(Instance instance, String namespaceName) throws NamespaceNotFoundException {
+ final ArrayList<Namespace.ID> singleId = new ArrayList<>(1);
+ populateMap(instance, (id, name) -> {
+ if (name.equals(namespaceName))
+ singleId.add(new Namespace.ID(id));
+ });
+ if (singleId.isEmpty())
+ throw new NamespaceNotFoundException(null, namespaceName, "getNamespaceId() failed to find namespace");
+ return singleId.get(0);
+ }
+
+ /**
+ * Look for namespace ID in ZK. Fail quietly by logging and returning null.
+ */
+ public static Namespace.ID lookupNamespaceId(Instance instance, String namespaceName) {
+ Namespace.ID id = null;
+ try {
+ id = getNamespaceId(instance, namespaceName);
+ } catch (NamespaceNotFoundException e) {
+ if (log.isDebugEnabled())
+ log.debug("Failed to find namespace ID from name: " + namespaceName, e);
+ }
+ return id;
+ }
+
+ /**
+ * Return true if namespace name exists
+ */
+ public static boolean namespaceNameExists(Instance instance, String namespaceName) {
+ return lookupNamespaceId(instance, namespaceName) != null;
+ }
+
+ /**
+ * Look for namespace name in ZK. Throw NamespaceNotFoundException if not found.
+ */
+ public static String getNamespaceName(Instance instance, Namespace.ID namespaceId) throws NamespaceNotFoundException {
+ final ArrayList<String> singleName = new ArrayList<>(1);
+ populateMap(instance, (id, name) -> {
--- End diff --
This is similar to what we had:
`String namespaceName = getIdToNameMap(instance).get(namespaceId);`
But uses the new populateMap method I created. I created the list so we could pass a mutable object to the BiConsumer. I didn't think a String would work since they are immutable. I could change this method back to operate the old way, if you think that is cleaner. It seems like some situations Java 8 code looks messier.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126746152
--- Diff: core/src/main/java/org/apache/accumulo/core/client/TableOfflineException.java ---
@@ -26,7 +27,7 @@ private static String getTableName(Instance instance, String tableId) {
if (tableId == null)
return " <unknown table> ";
try {
- String tableName = Tables.getTableName(instance, tableId);
+ String tableName = Tables.getTableName(instance, new Table.ID(tableId));
--- End diff --
> I think we should keep anything we do in this regard out of the public API
Yes, took me a moment to realize that this wasn't actually public facing. Still applies when viewed in a "it's a nice API to use"
> I imagine there was a good reason, but I don't have insight into the history of that part of KeyExtent and there don't appear to be any comments explaining why it was needed.
I know it was/is a big problem for them. Seeing the prevalence of this change, I assume it would also be non-trivial for us..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126745375
--- Diff: core/src/main/java/org/apache/accumulo/core/client/TableOfflineException.java ---
@@ -26,7 +27,7 @@ private static String getTableName(Instance instance, String tableId) {
if (tableId == null)
return " <unknown table> ";
try {
- String tableName = Tables.getTableName(instance, tableId);
+ String tableName = Tables.getTableName(instance, new Table.ID(tableId));
--- End diff --
I found the code where we were doing this for `String` tableIds. It's in `KeyExtent`. I'm not sure if this was a case of premature optimization, or if the GC was an issue in the case of parsing the metadata table or something. I imagine there was a good reason, but I don't have insight into the history of that part of `KeyExtent` and there don't appear to be any comments explaining why it was needed.
For now, I think we should keep anything we do in this regard out of the public API. I don't think users need to worry about this optimization. If the object deduplication in `KeyExtent` is still valid, this can be pushed down to the `Table.ID` and `Namespace.ID` classes, replacing the optimization in `KeyExtent`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126531695
--- Diff: core/src/main/java/org/apache/accumulo/core/client/TableOfflineException.java ---
@@ -26,7 +27,7 @@ private static String getTableName(Instance instance, String tableId) {
if (tableId == null)
return " <unknown table> ";
try {
- String tableName = Tables.getTableName(instance, tableId);
+ String tableName = Tables.getTableName(instance, new Table.ID(tableId));
--- End diff --
I like this idea. I can definitely create a follow on ticket.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126750616
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+ }
+
+ @Override
+ public int hashCode() {
+ if (hashCode == null) {
+ hashCode = Objects.hash(canonicalID());
+ }
+ return hashCode;
+ }
+
+ /**
+ * Returns a string of the canonical ID
+ */
+ @Override
+ public String toString() {
+ return canonical;
+ }
+
+ /**
+ * Return a UTF_8 byte[] of the canonical ID.
+ */
+ public final byte[] getUtf8Bytes() {
+ return canonical.getBytes(UTF_8);
--- End diff --
OK, there's no reason for us not to use UTF-8 everywhere that I know of. I'd suggest making this `getBytes()` and rely on the javadoc as-is to inform callers about what the bytes "are".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126757396
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespace.java ---
@@ -0,0 +1,42 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+
+public class Namespace {
--- End diff --
@milleruntime took based this implementation on my [ACCUMULO-2589] branch, as I had suggested in the comments of this JIRA issue ([ACCUMULO-3238]). The longer term goal with my branch was to make `Table` and `Namespace` first class citizens in the public API. The ID would be just one facet of those API classes. At this point, it doesn't matter, because they are being used only for internal code. However, I do think it'd be easier for follow-on work to keep the inner-class, in case more public-facing features are adapted from my branch later.
[ACCUMULO-2589]: https://issues.apache.org/jira/browse/ACCUMULO-2589
[ACCUMULO-3238]: https://issues.apache.org/jira/browse/ACCUMULO-3238
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126740368
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import org.apache.accumulo.core.client.Instance;
+import org.apache.hadoop.io.Text;
+
+public class Table {
+
+ /**
+ * Object representing an internal table ID. This class was created to help with type safety. For help obtaining the value of a table ID from Zookeeper, see
+ * {@link Tables#getTableId(Instance, String)}
+ */
+ public static class ID extends AbstractId {
+ private static final long serialVersionUID = 7399913185860577809L;
+
+ public static final ID METADATA = new ID("!0");
+ public static final ID REPLICATION = new ID("+rep");
+ public static final ID ROOT = new ID("+r");
+
+ public static ID empty() {
+ return new ID("");
--- End diff --
Make this a singleton.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126750554
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
--- End diff --
Once in `getNameToIdMap` and once when streaming, yeah, but I don't think it's that big of an issue to transform the internal form to the API/presentation form. The set of tables is relatively small.
An option which wouldn't require it to iterate twice would be to make `getNameToIdMap` a generic method, which accepts a `Function` so it can transform to the value to the requested form:
```java
return Tables.getNameToIdMap(e -> e.canonicalId());
```
Tables.java:
```java
public <T> SortedMap<String, T> getNameToIdMap(Function<Table.ID, ? extends T> valueConverter) {
// ...
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/279
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126507505
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
public static String getNamespaceName(Instance instance, String namespaceId) throws NamespaceNotFoundException {
String namespaceName = getIdToNameMap(instance).get(namespaceId);
if (namespaceName == null)
throw new NamespaceNotFoundException(namespaceId, null, "getNamespaceName() failed to find namespace");
return namespaceName;
}
+ /**
+ * @deprecated Do not use - Not type safe, ambiguous String-String map. Use {@link #getNameMap(Instance)}
+ */
+ @Deprecated
--- End diff --
I see the name was changed in order to preserve the old method as deprecated, but since it can be removed in favor of the new method (no need to deprecate internal-only APIs), the original name can be preserved with the new behavior. Granted, the name isn't spectacularly clever, but it was very convenient in that it was very descriptive.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126507110
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---
@@ -100,50 +106,150 @@ private static ZooCache getZooCache(Instance instance) {
return namespaceMap;
}
- public static boolean exists(Instance instance, String namespaceId) {
+ public static boolean exists(Instance instance, Namespace.ID namespaceId) {
ZooCache zc = getZooCache(instance);
List<String> namespaceIds = zc.getChildren(ZooUtil.getRoot(instance) + Constants.ZNAMESPACES);
- return namespaceIds.contains(namespaceId);
- }
-
- public static String getNamespaceId(Instance instance, String namespace) throws NamespaceNotFoundException {
- String id = getNameToIdMap(instance).get(namespace);
- if (id == null)
- throw new NamespaceNotFoundException(null, namespace, "getNamespaceId() failed to find namespace");
- return id;
+ return namespaceIds.contains(namespaceId.canonicalID());
}
+ /**
+ * @deprecated Do not use - String for namespace ID is not type safe. Use {@link #getNamespaceName(Instance, Namespace.ID)}
+ */
+ @Deprecated
--- End diff --
This is internal only. We don't need to deprecate it. We can just remove it. Same with other deprecations below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/279#discussion_r126501018
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/AbstractId.java ---
@@ -0,0 +1,86 @@
+/*
+ * 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
+ *
+ * http://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.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+/**
+ * An abstract identifier class for comparing equality of identifiers of the same type.
+ */
+public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+
+ private static final long serialVersionUID = -155513612834787244L;
+ private final String canonical;
+ private Integer hashCode = null;
+
+ protected AbstractId(final String canonical) {
+ requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = canonical;
+ }
+
+ /**
+ * The canonical ID
+ */
+ public final String canonicalID() {
+ return canonical;
+ }
+
+ public boolean isEmpty() {
+ return canonical.isEmpty();
+ }
+
+ /**
+ * AbstractID objects are considered equal if, and only if, they are of the same type and have the same canonical identifier.
+ */
+ @Override
+ public boolean equals(final Object obj) {
+ return obj != null && Objects.equals(getClass(), obj.getClass()) && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+ }
+
+ @Override
+ public int hashCode() {
+ if (hashCode == null) {
+ hashCode = Objects.hash(canonicalID());
+ }
+ return hashCode;
+ }
+
+ /**
+ * Returns a string of the canonical ID
+ */
+ @Override
+ public String toString() {
+ return canonical;
+ }
+
+ /**
+ * Return a UTF_8 byte[] of the canonical ID.
+ */
+ public final byte[] getUtf8Bytes() {
+ return canonical.getBytes(UTF_8);
+ }
+
+ public int compareTo(AbstractId id) {
--- End diff --
Missing Override annotation?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---