[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

classic Classic list List threaded Threaded
66 messages Options
1234
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
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 <[hidden email]>
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #279: ACCUMULO-3238 Table.ID Namespace.ID Refactor

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
1234
Loading...