[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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

[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

ctubbsii
GitHub user milleruntime opened a pull request:

    https://github.com/apache/accumulo/pull/292

    ACCUMULO-4681 Created Table & Namespace WeakReference map

    Moved the de-dupe ID code out of KeyExtent to Table and Namespace.  Created "of" static methods to access ID objects stored in internal WeakHashMap.  Replaced all occurrences of "new ID" with static "of" methods.  Created 2 new tests for these changes: NamespaceTest and TableTest.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/milleruntime/accumulo ACCUMULO-4681

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/292.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 #292
   
----
commit aed9240412a581c0ecadb1aace5463311b11dda2
Author: Mike Miller <[hidden email]>
Date:   2017-08-04T21:44:58Z

    ACCUMULO-4681 Created Table & Namespace WeakReference map

----


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/292#discussion_r132822652
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
     
    -    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 final ID METADATA = of("!0");
    +    public static final ID REPLICATION = of("+rep");
    +    public static final ID ROOT = of("+r");
     
    -    public ID(final String canonical) {
    +    private ID(final String canonical) {
           super(canonical);
         }
    +
    +    /**
    +     * Get a Table.ID object for the provided canonical string.
    +     *
    +     * @param canonical
    +     *          table ID string
    +     * @return Table.ID object
    +     */
    +    public static Table.ID of(final String canonical) {
    +      return dedupeTableId(canonical);
    +    }
    +
    +    private static Table.ID dedupeTableId(String tableIdString) {
    --- End diff --
   
    Might be able to move the dedupe logic to the AbstractId class, with a generic method for the ID type. Would need to pass in the WeakHashMap as a parameter, and possibly a pre-constructed instance (to avoid reflection) to use if the map does not contain that ID. May not be consolidating this logic.


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r132823444
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
    --- End diff --
   
    It occurs to me that because of Java string interning, using a String as a key may mean that entries are kept around longer than necessary. The `WeakReference<Table.ID>` will still remove unused `Table.ID` objects and it will dedupe when it does exist, but the map entry might stick around unnecessarily because the String still has a strong reference because it's used in more places than just the Table.ID. This could be avoided by always constructing a new String and using that as the canonical parameter and the map key... but not sure it's worth it.


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r132825236
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
     
    -    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 final ID METADATA = of("!0");
    +    public static final ID REPLICATION = of("+rep");
    +    public static final ID ROOT = of("+r");
     
    -    public ID(final String canonical) {
    +    private ID(final String canonical) {
           super(canonical);
         }
    +
    +    /**
    +     * Get a Table.ID object for the provided canonical string.
    +     *
    +     * @param canonical
    +     *          table ID string
    +     * @return Table.ID object
    +     */
    +    public static Table.ID of(final String canonical) {
    +      return dedupeTableId(canonical);
    +    }
    +
    +    private static Table.ID dedupeTableId(String tableIdString) {
    +      Table.ID tableId;
    +      synchronized (tableIds) {
    +        WeakReference<Table.ID> tableIdRef = tableIds.get(tableIdString);
    +        if (tableIdRef != null) {
    +          tableId = tableIdRef.get();
    +          if (tableId != null) {
    +            return tableId;
    +          }
    +        }
    +
    +        tableId = new ID(tableIdString);
    +        tableIds.put(tableIdString, new WeakReference<>(tableId));
    --- End diff --
   
    I was thinking through the logic of this put method (and it's "compute" alternatives), and realized that there's no way to get a strong reference to the key itself from the map. You can only do comparisons. So, there's a chance that when you do a "put" here, it will reuse the old key, but the new value (in the case where the map entry exists, but the WeakReference is empty). The result is that the entry could disappear from the map even though the object in the WeakReference is still being used. In this case, we lose deduplication. There does not appear to be a solution to this (short of writing our own map class, or iterating over the keyset and comparing keys ourselves), and our deduplication is "best effort", so it's not a big deal... but it's worth keeping in mind, so we don't even try to use this paradigm for guaranteed deduplication.


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r132825421
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
     
    -    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 final ID METADATA = of("!0");
    +    public static final ID REPLICATION = of("+rep");
    +    public static final ID ROOT = of("+r");
     
    -    public ID(final String canonical) {
    +    private ID(final String canonical) {
           super(canonical);
         }
    +
    +    /**
    +     * Get a Table.ID object for the provided canonical string.
    +     *
    +     * @param canonical
    +     *          table ID string
    +     * @return Table.ID object
    +     */
    +    public static Table.ID of(final String canonical) {
    +      return dedupeTableId(canonical);
    +    }
    +
    +    private static Table.ID dedupeTableId(String tableIdString) {
    +      Table.ID tableId;
    +      synchronized (tableIds) {
    +        WeakReference<Table.ID> tableIdRef = tableIds.get(tableIdString);
    +        if (tableIdRef != null) {
    +          tableId = tableIdRef.get();
    +          if (tableId != null) {
    +            return tableId;
    +          }
    +        }
    +
    +        tableId = new ID(tableIdString);
    +        tableIds.put(tableIdString, new WeakReference<>(tableId));
    --- End diff --
   
    To clarify what I mean, "put" doesn't replace a key if it already exists. It only updates the value. This also might be mitigated by using Table.ID as the key, since it's less likely that the Table.ID will exist as a key in the map and point to an empty WeakReference.


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r132972878
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
    --- End diff --
   
    I did see this happen while tinkering around with the Tests.  The keys were staying around but the WeakReference value was null.


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r132999867
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
     
    -    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 final ID METADATA = of("!0");
    +    public static final ID REPLICATION = of("+rep");
    +    public static final ID ROOT = of("+r");
     
    -    public ID(final String canonical) {
    +    private ID(final String canonical) {
           super(canonical);
         }
    +
    +    /**
    +     * Get a Table.ID object for the provided canonical string.
    +     *
    +     * @param canonical
    +     *          table ID string
    +     * @return Table.ID object
    +     */
    +    public static Table.ID of(final String canonical) {
    +      return dedupeTableId(canonical);
    +    }
    +
    +    private static Table.ID dedupeTableId(String tableIdString) {
    --- End diff --
   
    I thought about this but it would require reflection or passing a pre-constructed instance object as a parameter to the method.  I wanted to avoid reflection since we lose some of the type safety. I also wanted to avoid created objects every time for parameters since they may not be needed if the map already contains the id.  
   
    Passing in a constructor object as a parameter may be a nice compromise but this introduces a bunch of potential exceptions:
    <pre>
    try {
            id = constructor.newInstance(idString);
          } catch (InstantiationException e) {
            e.printStackTrace();
          } catch (IllegalAccessException e) {
            e.printStackTrace();
          } catch (InvocationTargetException e) {
            e.printStackTrace();
          }
          idHashMap.put(idString, new WeakReference<>(id));
    </pre>


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r133002408
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
     
    -    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 final ID METADATA = of("!0");
    +    public static final ID REPLICATION = of("+rep");
    +    public static final ID ROOT = of("+r");
     
    -    public ID(final String canonical) {
    +    private ID(final String canonical) {
           super(canonical);
         }
    +
    +    /**
    +     * Get a Table.ID object for the provided canonical string.
    +     *
    +     * @param canonical
    +     *          table ID string
    +     * @return Table.ID object
    +     */
    +    public static Table.ID of(final String canonical) {
    +      return dedupeTableId(canonical);
    +    }
    +
    +    private static Table.ID dedupeTableId(String tableIdString) {
    +      Table.ID tableId;
    +      synchronized (tableIds) {
    +        WeakReference<Table.ID> tableIdRef = tableIds.get(tableIdString);
    +        if (tableIdRef != null) {
    +          tableId = tableIdRef.get();
    +          if (tableId != null) {
    +            return tableId;
    +          }
    +        }
    +
    +        tableId = new ID(tableIdString);
    +        tableIds.put(tableIdString, new WeakReference<>(tableId));
    --- End diff --
   
    Perhaps a WeakHashMap isn't the correct data structure for our situation.  Aren't we trying to simply cache IDs to prevent unnecessary object creation?  Perhaps a cache is all we want.


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r133002554
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
     
    -    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 final ID METADATA = of("!0");
    +    public static final ID REPLICATION = of("+rep");
    +    public static final ID ROOT = of("+r");
     
    -    public ID(final String canonical) {
    +    private ID(final String canonical) {
           super(canonical);
         }
    +
    +    /**
    +     * Get a Table.ID object for the provided canonical string.
    +     *
    +     * @param canonical
    +     *          table ID string
    +     * @return Table.ID object
    +     */
    +    public static Table.ID of(final String canonical) {
    +      return dedupeTableId(canonical);
    +    }
    +
    +    private static Table.ID dedupeTableId(String tableIdString) {
    +      Table.ID tableId;
    +      synchronized (tableIds) {
    +        WeakReference<Table.ID> tableIdRef = tableIds.get(tableIdString);
    +        if (tableIdRef != null) {
    +          tableId = tableIdRef.get();
    +          if (tableId != null) {
    +            return tableId;
    +          }
    +        }
    +
    +        tableId = new ID(tableIdString);
    +        tableIds.put(tableIdString, new WeakReference<>(tableId));
    --- End diff --
   
    These comments made me think this: https://stackoverflow.com/questions/1802809/javas-weakhashmap-and-caching-why-is-it-referencing-the-keys-not-the-values/1803213


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r133059580
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
     
    -    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 final ID METADATA = of("!0");
    +    public static final ID REPLICATION = of("+rep");
    +    public static final ID ROOT = of("+r");
     
    -    public ID(final String canonical) {
    +    private ID(final String canonical) {
           super(canonical);
         }
    +
    +    /**
    +     * Get a Table.ID object for the provided canonical string.
    +     *
    +     * @param canonical
    +     *          table ID string
    +     * @return Table.ID object
    +     */
    +    public static Table.ID of(final String canonical) {
    +      return dedupeTableId(canonical);
    +    }
    +
    +    private static Table.ID dedupeTableId(String tableIdString) {
    --- End diff --
   
    Passing in a `Function<String,T>` might be a nice compromise. Or a `Supplier<T>`:
   
    ```java
    protected static <T extends AbstractId> T dedupe(WeakHashMap<String,WeakReference<T>> cache, Supplier<T> newInstanceFunction) {
        //....
    }
    ```
    In Table.ID:
    ```java
    public ID of(String canonical) {
        return dedupe(cache, () -> new ID(canonical));
    }
    ```
   
    Shared implementation. No reflection. No unnecessary construction.


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r133068592
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
     
    -    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 final ID METADATA = of("!0");
    +    public static final ID REPLICATION = of("+rep");
    +    public static final ID ROOT = of("+r");
     
    -    public ID(final String canonical) {
    +    private ID(final String canonical) {
           super(canonical);
         }
    +
    +    /**
    +     * Get a Table.ID object for the provided canonical string.
    +     *
    +     * @param canonical
    +     *          table ID string
    +     * @return Table.ID object
    +     */
    +    public static Table.ID of(final String canonical) {
    +      return dedupeTableId(canonical);
    +    }
    +
    +    private static Table.ID dedupeTableId(String tableIdString) {
    +      Table.ID tableId;
    +      synchronized (tableIds) {
    +        WeakReference<Table.ID> tableIdRef = tableIds.get(tableIdString);
    +        if (tableIdRef != null) {
    +          tableId = tableIdRef.get();
    +          if (tableId != null) {
    +            return tableId;
    +          }
    +        }
    +
    +        tableId = new ID(tableIdString);
    +        tableIds.put(tableIdString, new WeakReference<>(tableId));
    --- End diff --
   
    Talked to @keith-turner about this briefly. I think the `WeakHashMap<String,WeakReference<>>` is the right type, but the String that's used as a key should be a new one, so its presence in the map is not dependent on the caller's strong reference to the key, but on the caller's strong reference to the returned Table.ID object. In other words, it should be:
   
    ```java
        T ret;
        WeakReference<T> ref = cache.get(canonical);
        if (ref == null || (ret = ref.get()) == null) {
            String s = new String(canonical);
            cache.put(s, ret = new Table.ID(s));
        }
        return ret;
    ```
   
    That way, the new String is only contained inside the ID and its presence in the map is contingent on the ID object still having a strong reference to it. Of course, the user could call `ID.getCanonical()` and get a strong reference to the key, thus holding it in the map even when the ID object is gone. This could be avoided by the constructor creating a second copy... the first is held by the ID only for the purposes of having a strong reference to the map key, and the second would be returned in `.getCanonical()` and `.toString()`.


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r133070150
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/Table.java ---
    @@ -16,24 +16,59 @@
      */
     package org.apache.accumulo.core.client.impl;
     
    +import java.lang.ref.WeakReference;
    +import java.util.WeakHashMap;
    +
     import org.apache.accumulo.core.client.Instance;
     
     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)}
    +   *
    +   * Uses an internal WeakHashMap and private constructor for storing a WeakReference of every Table.ID. Therefore, a Table.ID can't be instantiated outside
    +   * this class and is accessed by calling Table.ID.{@link #of(String)}.
        */
       public static class ID extends AbstractId {
         private static final long serialVersionUID = 7399913185860577809L;
    +    static final WeakHashMap<String,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
     
    -    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 final ID METADATA = of("!0");
    +    public static final ID REPLICATION = of("+rep");
    +    public static final ID ROOT = of("+r");
     
    -    public ID(final String canonical) {
    +    private ID(final String canonical) {
           super(canonical);
         }
    +
    +    /**
    +     * Get a Table.ID object for the provided canonical string.
    +     *
    +     * @param canonical
    +     *          table ID string
    +     * @return Table.ID object
    +     */
    +    public static Table.ID of(final String canonical) {
    +      return dedupeTableId(canonical);
    +    }
    +
    +    private static Table.ID dedupeTableId(String tableIdString) {
    +      Table.ID tableId;
    +      synchronized (tableIds) {
    +        WeakReference<Table.ID> tableIdRef = tableIds.get(tableIdString);
    +        if (tableIdRef != null) {
    +          tableId = tableIdRef.get();
    +          if (tableId != null) {
    +            return tableId;
    +          }
    +        }
    +
    +        tableId = new ID(tableIdString);
    +        tableIds.put(tableIdString, new WeakReference<>(tableId));
    --- End diff --
   
    The main point is that these objects, even the extra String instances, would only be created when needed. Usually, the previous instance would be used. Still can't avoid the potential duplicates I mentioned earlier, unless you force removal of the old value before calling `put` in the case where `ref != null && ref.get() == null`, but the new String instances should dramatically decrease the liklihood of this happening, and a few dupes are probably okay.


---
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 issue #292: ACCUMULO-4681 Created Table & Namespace WeakReference m...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/292
 
    This whole thing might be easier to implement with [`CacheBuilder.weakValues()`][1] from Guava. Eviction seems to occur if value disappears. I **think** this will do what we want, and it should automatically hide the WeakReference stuff from us, auto-evicting stuff when the value is gone so we never see it as present in the map. The key can be String, without the need to do any crazy `new String` stuff, or early construction. It ensures a dependency on Guava, though.
   
    [1]: https://google.github.io/guava/releases/16.0/api/docs/com/google/common/cache/CacheBuilder.html#weakValues()


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

ctubbsii
In reply to this post by ctubbsii
Github user mjwall commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/292#discussion_r133224225
 
    --- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/TableTest.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * 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 org.junit.Assert.assertEquals;
    +import static org.junit.Assert.assertTrue;
    +import static org.junit.Assert.fail;
    +
    +import org.junit.After;
    +import org.junit.Test;
    +
    +/**
    + * Tests the Table ID class, mainly the internal WeakHashMap.
    + */
    +public class TableTest {
    +
    +  @After
    +  public void cleanup() {
    +    garbageCollect(Table.ID.tableIds.size());
    +  }
    +
    +  @Test
    +  public void testWeakHashMapIncreases() {
    +    // ensure initial size contains just the built in Table IDs (metadata, root, replication)
    +    assertEquals(3, Table.ID.tableIds.size());
    +
    +    String tableString = new String("table-testWeakHashMapIncreases");
    +    Table.ID table1 = Table.ID.of(tableString);
    +    assertEquals(4, Table.ID.tableIds.size());
    +    assertEquals(tableString, table1.canonicalID());
    +  }
    +
    +  @Test
    +  public void testWeakHashMapNoDuplicates() {
    +    String tableString = new String("table-testWeakHashMapNoDuplicates");
    +    Table.ID table1 = Table.ID.of(tableString);
    +    assertEquals(4, Table.ID.tableIds.size());
    +    assertEquals(tableString, table1.canonicalID());
    +
    +    // ensure duplicates are not created
    +    Table.ID builtInTableId = Table.ID.of("!0");
    +    assertEquals(Table.ID.METADATA, builtInTableId);
    +    builtInTableId = Table.ID.of("+r");
    +    assertEquals(Table.ID.ROOT, builtInTableId);
    +    builtInTableId = Table.ID.of("+rep");
    +    assertEquals(Table.ID.REPLICATION, builtInTableId);
    +    table1 = Table.ID.of(tableString);
    +    assertEquals(4, Table.ID.tableIds.size());
    +    assertEquals(tableString, table1.canonicalID());
    +  }
    +
    +  @Test
    +  public void testWeakHashMapDecreases() {
    +    // get bunch of table IDs that are not used
    +    for (int i = 0; i < 1000; i++)
    +      Table.ID.of(new String("table" + i));
    --- End diff --
   
    what is this testing?


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

ctubbsii
In reply to this post by ctubbsii
Github user mjwall commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/292#discussion_r133224400
 
    --- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/TableTest.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * 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 org.junit.Assert.assertEquals;
    +import static org.junit.Assert.assertTrue;
    +import static org.junit.Assert.fail;
    +
    +import org.junit.After;
    +import org.junit.Test;
    +
    +/**
    + * Tests the Table ID class, mainly the internal WeakHashMap.
    + */
    +public class TableTest {
    +
    +  @After
    +  public void cleanup() {
    +    garbageCollect(Table.ID.tableIds.size());
    +  }
    +
    +  @Test
    +  public void testWeakHashMapIncreases() {
    +    // ensure initial size contains just the built in Table IDs (metadata, root, replication)
    +    assertEquals(3, Table.ID.tableIds.size());
    +
    +    String tableString = new String("table-testWeakHashMapIncreases");
    +    Table.ID table1 = Table.ID.of(tableString);
    +    assertEquals(4, Table.ID.tableIds.size());
    +    assertEquals(tableString, table1.canonicalID());
    +  }
    +
    +  @Test
    +  public void testWeakHashMapNoDuplicates() {
    +    String tableString = new String("table-testWeakHashMapNoDuplicates");
    +    Table.ID table1 = Table.ID.of(tableString);
    +    assertEquals(4, Table.ID.tableIds.size());
    +    assertEquals(tableString, table1.canonicalID());
    +
    +    // ensure duplicates are not created
    +    Table.ID builtInTableId = Table.ID.of("!0");
    +    assertEquals(Table.ID.METADATA, builtInTableId);
    +    builtInTableId = Table.ID.of("+r");
    +    assertEquals(Table.ID.ROOT, builtInTableId);
    +    builtInTableId = Table.ID.of("+rep");
    +    assertEquals(Table.ID.REPLICATION, builtInTableId);
    +    table1 = Table.ID.of(tableString);
    +    assertEquals(4, Table.ID.tableIds.size());
    +    assertEquals(tableString, table1.canonicalID());
    +  }
    +
    +  @Test
    +  public void testWeakHashMapDecreases() {
    +    // get bunch of table IDs that are not used
    +    for (int i = 0; i < 1000; i++)
    +      Table.ID.of(new String("table" + i));
    --- End diff --
   
    oh, I see it in the cleanup


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r133225358
 
    --- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/TableTest.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * 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 org.junit.Assert.assertEquals;
    +import static org.junit.Assert.assertTrue;
    +import static org.junit.Assert.fail;
    +
    +import org.junit.After;
    +import org.junit.Test;
    +
    +/**
    + * Tests the Table ID class, mainly the internal WeakHashMap.
    + */
    +public class TableTest {
    +
    +  @After
    +  public void cleanup() {
    +    garbageCollect(Table.ID.tableIds.size());
    +  }
    +
    +  @Test
    +  public void testWeakHashMapIncreases() {
    +    // ensure initial size contains just the built in Table IDs (metadata, root, replication)
    +    assertEquals(3, Table.ID.tableIds.size());
    +
    +    String tableString = new String("table-testWeakHashMapIncreases");
    +    Table.ID table1 = Table.ID.of(tableString);
    +    assertEquals(4, Table.ID.tableIds.size());
    +    assertEquals(tableString, table1.canonicalID());
    +  }
    +
    +  @Test
    +  public void testWeakHashMapNoDuplicates() {
    +    String tableString = new String("table-testWeakHashMapNoDuplicates");
    +    Table.ID table1 = Table.ID.of(tableString);
    +    assertEquals(4, Table.ID.tableIds.size());
    +    assertEquals(tableString, table1.canonicalID());
    +
    +    // ensure duplicates are not created
    +    Table.ID builtInTableId = Table.ID.of("!0");
    +    assertEquals(Table.ID.METADATA, builtInTableId);
    +    builtInTableId = Table.ID.of("+r");
    +    assertEquals(Table.ID.ROOT, builtInTableId);
    +    builtInTableId = Table.ID.of("+rep");
    +    assertEquals(Table.ID.REPLICATION, builtInTableId);
    +    table1 = Table.ID.of(tableString);
    +    assertEquals(4, Table.ID.tableIds.size());
    +    assertEquals(tableString, table1.canonicalID());
    +  }
    +
    +  @Test
    +  public void testWeakHashMapDecreases() {
    +    // get bunch of table IDs that are not used
    +    for (int i = 0; i < 1000; i++)
    +      Table.ID.of(new String("table" + i));
    --- End diff --
   
    Yeah I found that was the best way to get the GC to collect.  I will add some comments since this isn't clear.


---
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 issue #292: ACCUMULO-4681 Created Table & Namespace WeakReference m...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/292
 
    > This whole thing might be easier to implement with CacheBuilder.weakValues() from Guava. Eviction seems to occur if value disappears. I think this will do what we want, and it should automatically hide the WeakReference stuff from us, auto-evicting stuff when the value is gone so we never see it as present in the map. The key can be String, without the need to do any crazy new String stuff, or early construction. It ensures a dependency on Guava, though.
   
    This is the way I was leaning yesterday after reading that stackoverflow response.  I think the issue with WeakHashMap is that they Keys are weak and not the values. So we have to add another layer of WeakRefernce for the values, complicating things.  In our case, it is just a cache and we really want weak values to store the ID objects.
   
    I also thought of still using WeakHashMap but flipping the keys and values - `WeakHashMap<AbstractId, String>.`  Using ID as the key so it will automatically be weak and then I guess the canonical String as the value.  But then the value is pretty useless.


---
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 issue #292: ACCUMULO-4681 Created Table & Namespace WeakReference m...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/292
 
    0f083e0c1793e6093e1fcd2fc18faaf8686e7af1 seems to be the most elegant solution.  Guava dependency is already in core so shouldn't pull in any new dependencies.  I will test momentarily if others agree.


---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r133335283
 
    --- Diff: server/master/src/main/java/org/apache/accumulo/master/tableOps/Utils.java ---
    @@ -59,21 +57,16 @@ static void checkTableDoesNotExist(Instance instance, String tableName, Table.ID
           throw new AcceptableThriftTableOperationException(null, tableName, operation, TableOperationExceptionType.EXISTS, null);
       }
     
    -  static <T extends AbstractId> T getNextTableId(String tableName, Instance instance, Class<T> idClassType) throws AcceptableThriftTableOperationException {
    +  static String getNextTableId(String tableName, Instance instance) throws AcceptableThriftTableOperationException {
    --- End diff --
   
    If you wanted to keep the original return type, you could pass in a function to map from String to ID:
   
    ```java
    static <T extends AbstractId> T getNextId(String name, Instance instance, Function<String, T> newIdFunction) throws AcceptableThriftTableOperationException {
        // ...
        return newIdFunction.apply(new String(nid, UTF_8));
    }
    ```
   
    Can use it like this:
    ```java
    Table.ID tableId = Util.getNextId(tableName, instance, Table.ID::of);
    ```



---
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 #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...

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/292#discussion_r133333221
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/data/impl/KeyExtent.java ---
    @@ -60,28 +58,11 @@
     
     public class KeyExtent implements WritableComparable<KeyExtent> {
     
    -  private static final WeakHashMap<Table.ID,WeakReference<Table.ID>> tableIds = new WeakHashMap<>();
    -
    -  private static Table.ID dedupeTableId(Table.ID tableId) {
    -    synchronized (tableIds) {
    -      WeakReference<Table.ID> etir = tableIds.get(tableId);
    -      if (etir != null) {
    -        Table.ID eti = etir.get();
    -        if (eti != null) {
    -          return eti;
    -        }
    -      }
    -
    -      tableIds.put(tableId, new WeakReference<>(tableId));
    -      return tableId;
    -    }
    -  }
    -
       private Table.ID tableId;
       private Text textEndRow;
       private Text textPrevEndRow;
     
    -  private final Table.ID EMPTY_ID = new Table.ID("");
    +  private final Table.ID EMPTY_ID = Table.ID.of("");
    --- End diff --
   
    This could be static.


---
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.
---
12
Loading...