You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by milleruntime <gi...@git.apache.org> on 2017/08/11 15:36:50 UTC
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
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 <mm...@apache.org>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #292: ACCUMULO-4681 Created Table & Namespace WeakReference m...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133695113
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -17,38 +17,73 @@
package org.apache.accumulo.core.client.impl;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.TestName;
/**
- * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ * Tests the Namespace ID class, mainly the internal cache.
*/
public class NamespaceTest {
+ @Rule
+ public TestName name = new TestName();
@Test
- public void testWeakHashMapIncreases() {
- String namespaceString = "namespace-testWeakHashMapIncreases";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheIncreases() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
}
@Test
- public void testWeakHashMapNoDuplicates() {
- String namespaceString = "namespace-testWeakHashMapNoDuplicates";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheNoDuplicates() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
// ensure duplicates are not created
Namespace.ID builtInNamespaceId = Namespace.ID.of("+accumulo");
- assertEquals(Namespace.ID.ACCUMULO, builtInNamespaceId);
+ assertSame(Namespace.ID.ACCUMULO, builtInNamespaceId);
builtInNamespaceId = Namespace.ID.of("+default");
- assertEquals(Namespace.ID.DEFAULT, builtInNamespaceId);
+ assertSame(Namespace.ID.DEFAULT, builtInNamespaceId);
nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
+ Namespace.ID nsId2 = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
+ assertSame(nsId, nsId2);
+ }
+
+ @Test(timeout = 60_000)
+ public void testCacheDecreasesAfterGC() {
+ generateJunkCacheEntries();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
--- End diff --
Ah yeah good thinking.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133582940
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -17,38 +17,73 @@
package org.apache.accumulo.core.client.impl;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.TestName;
/**
- * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ * Tests the Namespace ID class, mainly the internal cache.
*/
public class NamespaceTest {
+ @Rule
+ public TestName name = new TestName();
@Test
- public void testWeakHashMapIncreases() {
- String namespaceString = "namespace-testWeakHashMapIncreases";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheIncreases() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
}
@Test
- public void testWeakHashMapNoDuplicates() {
- String namespaceString = "namespace-testWeakHashMapNoDuplicates";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheNoDuplicates() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
// ensure duplicates are not created
Namespace.ID builtInNamespaceId = Namespace.ID.of("+accumulo");
- assertEquals(Namespace.ID.ACCUMULO, builtInNamespaceId);
+ assertSame(Namespace.ID.ACCUMULO, builtInNamespaceId);
builtInNamespaceId = Namespace.ID.of("+default");
- assertEquals(Namespace.ID.DEFAULT, builtInNamespaceId);
+ assertSame(Namespace.ID.DEFAULT, builtInNamespaceId);
nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
+ Namespace.ID nsId2 = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
+ assertSame(nsId, nsId2);
+ }
+
+ @Test(timeout = 60_000)
+ public void testCacheDecreasesAfterGC() {
+ generateJunkCacheEntries();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
--- End diff --
This could have already garbage collected some of the entries, after leaving the method, so further GC may not do anything. Instead, consider setting the initialSize before the junk entries are added, and the loop condition should be: `while (postGCSize > initialSize);`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by mjwall <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #292: ACCUMULO-4681 Created Table & Namespace WeakReference m...
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133334428
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.junit.Test;
+
+/**
+ * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ */
+public class NamespaceTest {
+
+ @Test
+ public void testWeakHashMapIncreases() {
+ String namespaceString = "namespace-testWeakHashMapIncreases";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+ }
+
+ @Test
+ public void testWeakHashMapNoDuplicates() {
+ String namespaceString = "namespace-testWeakHashMapNoDuplicates";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+
+ // ensure duplicates are not created
+ Namespace.ID builtInNamespaceId = Namespace.ID.of("+accumulo");
+ assertEquals(Namespace.ID.ACCUMULO, builtInNamespaceId);
+ builtInNamespaceId = Namespace.ID.of("+default");
+ assertEquals(Namespace.ID.DEFAULT, builtInNamespaceId);
+ nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+ }
+}
--- End diff --
Could add a small test to ensure size decreases once strong refs go away. Might not be able to use `.size()` because Cache may not clean up immediately, but should be able to use `.stream().count()` to get an accurate size of remaining elements after GC.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime closed the pull request at:
https://github.com/apache/accumulo/pull/292
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133333857
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.junit.Test;
+
+/**
+ * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ */
+public class NamespaceTest {
+
+ @Test
+ public void testWeakHashMapIncreases() {
+ String namespaceString = "namespace-testWeakHashMapIncreases";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+ }
+
+ @Test
+ public void testWeakHashMapNoDuplicates() {
+ String namespaceString = "namespace-testWeakHashMapNoDuplicates";
--- End diff --
Could guarantee these strings are unique using JUnit `@Rule TestName`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133503470
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.junit.Test;
+
+/**
+ * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ */
+public class NamespaceTest {
+
+ @Test
+ public void testWeakHashMapIncreases() {
+ String namespaceString = "namespace-testWeakHashMapIncreases";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+ }
+
+ @Test
+ public void testWeakHashMapNoDuplicates() {
+ String namespaceString = "namespace-testWeakHashMapNoDuplicates";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+
+ // ensure duplicates are not created
+ Namespace.ID builtInNamespaceId = Namespace.ID.of("+accumulo");
+ assertEquals(Namespace.ID.ACCUMULO, builtInNamespaceId);
+ builtInNamespaceId = Namespace.ID.of("+default");
+ assertEquals(Namespace.ID.DEFAULT, builtInNamespaceId);
--- End diff --
Cool. I've never used `assertSame`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133528726
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.junit.Test;
+
+/**
+ * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ */
+public class NamespaceTest {
+
+ @Test
+ public void testWeakHashMapIncreases() {
+ String namespaceString = "namespace-testWeakHashMapIncreases";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+ }
+
+ @Test
+ public void testWeakHashMapNoDuplicates() {
+ String namespaceString = "namespace-testWeakHashMapNoDuplicates";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+
+ // ensure duplicates are not created
+ Namespace.ID builtInNamespaceId = Namespace.ID.of("+accumulo");
+ assertEquals(Namespace.ID.ACCUMULO, builtInNamespaceId);
+ builtInNamespaceId = Namespace.ID.of("+default");
+ assertEquals(Namespace.ID.DEFAULT, builtInNamespaceId);
+ nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+ }
+}
--- End diff --
I didn't have much luck measuring `.size()` or `asMap.size()` after GC using their cleanup method. I will try again using streams.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133333368
--- Diff: core/src/main/java/org/apache/accumulo/core/data/impl/KeyExtent.java ---
@@ -173,9 +154,9 @@ public KeyExtent(Text flattenedExtent, Text prevEndRow) {
public void setTableId(Table.ID tId) {
if (tId == null)
- throw new IllegalArgumentException("null table name not allowed");
+ throw new IllegalArgumentException("null table id not allowed");
--- End diff --
This could be `Objects.requiresNonNull(tId, "null table id not allowed")`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133333657
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.junit.Test;
+
+/**
+ * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ */
+public class NamespaceTest {
+
+ @Test
+ public void testWeakHashMapIncreases() {
+ String namespaceString = "namespace-testWeakHashMapIncreases";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
--- End diff --
Could also test that it does not increase after calling again with the same parameter (`Namespace.ID nsId2 = ....`), and that `assertSame(nsId, nsId2)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133530036
--- 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 --
Other than using the Function interface, this doesn't seem all that different from the reflection code that I just removed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133536502
--- 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 --
It's not different, but it prevents passing a String around (which I believe was the original reason you added the reflection code), but without reflection (lambdas are faster). It's up to you. I like the elegance of the functional stuff, but I don't think it matters much, as long as we're avoiding reflection.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133506281
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.junit.Test;
+
+/**
+ * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ */
+public class NamespaceTest {
+
+ @Test
+ public void testWeakHashMapIncreases() {
+ String namespaceString = "namespace-testWeakHashMapIncreases";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+ }
+
+ @Test
+ public void testWeakHashMapNoDuplicates() {
+ String namespaceString = "namespace-testWeakHashMapNoDuplicates";
--- End diff --
Cool never knew about this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133582248
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -17,38 +17,73 @@
package org.apache.accumulo.core.client.impl;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.TestName;
/**
- * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ * Tests the Namespace ID class, mainly the internal cache.
*/
public class NamespaceTest {
+ @Rule
+ public TestName name = new TestName();
@Test
- public void testWeakHashMapIncreases() {
- String namespaceString = "namespace-testWeakHashMapIncreases";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheIncreases() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
}
@Test
- public void testWeakHashMapNoDuplicates() {
- String namespaceString = "namespace-testWeakHashMapNoDuplicates";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheNoDuplicates() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
// ensure duplicates are not created
Namespace.ID builtInNamespaceId = Namespace.ID.of("+accumulo");
- assertEquals(Namespace.ID.ACCUMULO, builtInNamespaceId);
+ assertSame(Namespace.ID.ACCUMULO, builtInNamespaceId);
builtInNamespaceId = Namespace.ID.of("+default");
- assertEquals(Namespace.ID.DEFAULT, builtInNamespaceId);
+ assertSame(Namespace.ID.DEFAULT, builtInNamespaceId);
nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
+ Namespace.ID nsId2 = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
+ assertSame(nsId, nsId2);
+ }
+
+ @Test(timeout = 60_000)
+ public void testCacheDecreasesAfterGC() {
+ generateJunkCacheEntries();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
+ Long postGCSize;
+ System.out.println("Namespace.ID.cache.asMap().entrySet().stream().count() = " + initialSize);
+ do {
+ System.gc();
+ try {
+ Thread.sleep(5000);
--- End diff --
Could shorten this to 1 second or 500ms, since there's a good chance it the condition will be met quickly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by mjwall <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133333957
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.junit.Test;
+
+/**
+ * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ */
+public class NamespaceTest {
+
+ @Test
+ public void testWeakHashMapIncreases() {
+ String namespaceString = "namespace-testWeakHashMapIncreases";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+ }
+
+ @Test
+ public void testWeakHashMapNoDuplicates() {
+ String namespaceString = "namespace-testWeakHashMapNoDuplicates";
+ Integer initialSize = Namespace.ID.cache.asMap().size();
+ Namespace.ID nsId = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(namespaceString, nsId.canonicalID());
+
+ // ensure duplicates are not created
+ Namespace.ID builtInNamespaceId = Namespace.ID.of("+accumulo");
+ assertEquals(Namespace.ID.ACCUMULO, builtInNamespaceId);
+ builtInNamespaceId = Namespace.ID.of("+default");
+ assertEquals(Namespace.ID.DEFAULT, builtInNamespaceId);
--- End diff --
These should be `assertSame` instead of `assertEquals`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133695596
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -17,38 +17,73 @@
package org.apache.accumulo.core.client.impl;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.TestName;
/**
- * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ * Tests the Namespace ID class, mainly the internal cache.
*/
public class NamespaceTest {
+ @Rule
+ public TestName name = new TestName();
@Test
- public void testWeakHashMapIncreases() {
- String namespaceString = "namespace-testWeakHashMapIncreases";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheIncreases() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
}
@Test
- public void testWeakHashMapNoDuplicates() {
- String namespaceString = "namespace-testWeakHashMapNoDuplicates";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheNoDuplicates() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
// ensure duplicates are not created
Namespace.ID builtInNamespaceId = Namespace.ID.of("+accumulo");
- assertEquals(Namespace.ID.ACCUMULO, builtInNamespaceId);
+ assertSame(Namespace.ID.ACCUMULO, builtInNamespaceId);
builtInNamespaceId = Namespace.ID.of("+default");
- assertEquals(Namespace.ID.DEFAULT, builtInNamespaceId);
+ assertSame(Namespace.ID.DEFAULT, builtInNamespaceId);
nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
+ Namespace.ID nsId2 = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
+ assertSame(nsId, nsId2);
+ }
+
+ @Test(timeout = 60_000)
+ public void testCacheDecreasesAfterGC() {
+ generateJunkCacheEntries();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
+ Long postGCSize;
+ System.out.println("Namespace.ID.cache.asMap().entrySet().stream().count() = " + initialSize);
+ do {
+ System.gc();
+ try {
+ Thread.sleep(5000);
--- End diff --
My original GC test had a finite number of loops but that is a good point, no need to wait longer than needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #292: ACCUMULO-4681 Created Table & Namespace WeakReference m...
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133582097
--- Diff: core/src/test/java/org/apache/accumulo/core/client/impl/NamespaceTest.java ---
@@ -17,38 +17,73 @@
package org.apache.accumulo.core.client.impl;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.TestName;
/**
- * Tests the Namespace ID class, mainly the internal WeakHashMap.
+ * Tests the Namespace ID class, mainly the internal cache.
*/
public class NamespaceTest {
+ @Rule
+ public TestName name = new TestName();
@Test
- public void testWeakHashMapIncreases() {
- String namespaceString = "namespace-testWeakHashMapIncreases";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheIncreases() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
}
@Test
- public void testWeakHashMapNoDuplicates() {
- String namespaceString = "namespace-testWeakHashMapNoDuplicates";
- Integer initialSize = Namespace.ID.cache.asMap().size();
+ public void testCacheNoDuplicates() {
+ String namespaceString = "namespace-" + name.getMethodName();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
Namespace.ID nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
// ensure duplicates are not created
Namespace.ID builtInNamespaceId = Namespace.ID.of("+accumulo");
- assertEquals(Namespace.ID.ACCUMULO, builtInNamespaceId);
+ assertSame(Namespace.ID.ACCUMULO, builtInNamespaceId);
builtInNamespaceId = Namespace.ID.of("+default");
- assertEquals(Namespace.ID.DEFAULT, builtInNamespaceId);
+ assertSame(Namespace.ID.DEFAULT, builtInNamespaceId);
nsId = Namespace.ID.of(namespaceString);
- assertEquals(initialSize + 1, Namespace.ID.cache.asMap().size());
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
assertEquals(namespaceString, nsId.canonicalID());
+ Namespace.ID nsId2 = Namespace.ID.of(namespaceString);
+ assertEquals(initialSize + 1, Namespace.ID.cache.asMap().entrySet().stream().count());
+ assertSame(nsId, nsId2);
+ }
+
+ @Test(timeout = 60_000)
+ public void testCacheDecreasesAfterGC() {
+ generateJunkCacheEntries();
+ Long initialSize = Namespace.ID.cache.asMap().entrySet().stream().count();
+ Long postGCSize;
+ System.out.println("Namespace.ID.cache.asMap().entrySet().stream().count() = " + initialSize);
+ do {
+ System.gc();
+ try {
+ Thread.sleep(5000);
+ } catch (InterruptedException e) {
+ fail("Thread interrupted while waiting for GC");
+ }
+ postGCSize = Namespace.ID.cache.asMap().entrySet().stream().count();
+ System.out.println("After GC: Namespace.ID.cache.asMap().entrySet().stream().count() = " + postGCSize);
--- End diff --
Should use logger or remove these debugging lines.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #292: ACCUMULO-4681 Created Table & Namespace WeakRefe...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/292#discussion_r133335412
--- 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 {
try {
IZooReaderWriter zoo = ZooReaderWriter.getInstance();
final String ntp = ZooUtil.getRoot(instance) + Constants.ZTABLES;
- byte[] nid = zoo.mutate(ntp, ZERO_BYTE, ZooUtil.PUBLIC, new Mutator() {
- @Override
- public byte[] mutate(byte[] currentValue) throws Exception {
- BigInteger nextId = new BigInteger(new String(currentValue, UTF_8), Character.MAX_RADIX);
- nextId = nextId.add(BigInteger.ONE);
- return nextId.toString(Character.MAX_RADIX).getBytes(UTF_8);
- }
+ byte[] nid = zoo.mutate(ntp, ZERO_BYTE, ZooUtil.PUBLIC, currentValue -> {
+ BigInteger nextId = new BigInteger(new String(currentValue, UTF_8), Character.MAX_RADIX);
+ nextId = nextId.add(BigInteger.ONE);
+ return nextId.toString(Character.MAX_RADIX).getBytes(UTF_8);
});
- Constructor<T> constructor = idClassType.getConstructor(String.class);
- return constructor.newInstance(new String(nid, UTF_8));
--- End diff --
Very big +1 to getting rid of this reflection implementation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---