Accumulo-Pull-Requests - Build # 985 - Still Failing

[Apache Jenkins Server]

The Apache Jenkins build system has built Accumulo-Pull-Requests (build #985)

Status: Still Failing

Check console output at 
https://builds.apache.org/job/Accumulo-Pull-Requests/985/ to view the results.

[GitHub] keith-turner opened a new pull request #365: ACCUMULO-4779 Avoid locks in ZooCache when data in cache

[GitBox]

keith-turner opened a new pull request #365: ACCUMULO-4779 Avoid locks in 
ZooCache when data in cache
URL: https://github.com/apache/accumulo/pull/365
 
 
   ZooCache was using read and write locks.  For the case where lots
   of threads were accessing data present in the cache the read locks
   were really slowing things down.  This commit switches to immutable
   copies of all the data present in the cache which require no locks
   to access.  When the cache changes the immutable copies are
   regenerated and then made available.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ctubbsii commented on issue #315: ACCUMULO-4731 Improve exception handling if a key encryption key cannot be loaded

[GitBox]

ctubbsii commented on issue #315: ACCUMULO-4731 Improve exception handling if a 
key encryption key cannot be loaded
URL: https://github.com/apache/accumulo/pull/315#issuecomment-360311890
 
 
   Should ACCUMULO-4731 be closed now?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ctubbsii commented on issue #363: ACCUMULO-3361 CryptoTest creates files in /tmp/tmp

[GitBox]

ctubbsii commented on issue #363: ACCUMULO-3361 CryptoTest creates files in 
/tmp/tmp
URL: https://github.com/apache/accumulo/pull/363#issuecomment-360311714
 
 
   Test comment (should get copied to JIRA worklog)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ctubbsii commented on issue #363: ACCUMULO-3361 CryptoTest creates files in /tmp/tmp

[GitBox]

ctubbsii commented on issue #363: ACCUMULO-3361 CryptoTest creates files in 
/tmp/tmp
URL: https://github.com/apache/accumulo/pull/363#issuecomment-359932393
 
 
   Test comment (should get copied to JIRA worklog)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ctubbsii commented on issue #363: ACCUMULO-3361 CryptoTest creates files in /tmp/tmp

[GitBox]

ctubbsii commented on issue #363: ACCUMULO-3361 CryptoTest creates files in 
/tmp/tmp
URL: https://github.com/apache/accumulo/pull/363#issuecomment-360311714
 
 
   Test comment (should get copied to JIRA worklog)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Accumulo-Pull-Requests - Build # 984 - Failure

[Apache Jenkins Server]

The Apache Jenkins build system has built Accumulo-Pull-Requests (build #984)

Status: Failure

Check console output at 
https://builds.apache.org/job/Accumulo-Pull-Requests/984/ to view the results.

[GitHub] milleruntime commented on issue #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

milleruntime commented on issue #364: ACCUMULO-4778 Initial feedback for table 
name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#issuecomment-360302640
 
 
   Changes so far are causing errors in TCompactProxyIT because it reuses table 
names.  I think we need a unit test to pin point where exactly the cache needs 
to cleared where it did not have to previously.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Accumulo-Pull-Requests - Build # 983 - Unstable

[Apache Jenkins Server]

The Apache Jenkins build system has built Accumulo-Pull-Requests (build #983)

Status: Unstable

Check console output at 
https://builds.apache.org/job/Accumulo-Pull-Requests/983/ to view the results.

[GitHub] milleruntime commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

milleruntime commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163631918
 
 

 ##
 File path: 
core/src/main/java/org/apache/accumulo/core/client/impl/TableMap.java
 ##
 @@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.core.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.client.impl.Tables.getZooCache;
+import static org.apache.accumulo.core.client.impl.Tables.qualified;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.client.Instance;
+import org.apache.accumulo.core.client.NamespaceNotFoundException;
+import org.apache.accumulo.core.zookeeper.ZooUtil;
+import org.apache.accumulo.fate.zookeeper.ZooCache;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.collect.ImmutableMap;
+
+/**
+ * Used for thread safe caching of table ID maps
+ */
+public class TableMap {
+  private static final Logger log = LoggerFactory.getLogger(TableMap.class);
+
+  private Map tableNameToIdMap;
+  private Map tableIdtoNameMap;
+
+  public TableMap(Instance instance) {
+generateMaps(instance);
+  }
+
+  private void generateMaps(Instance instance) {
+ZooCache zc = getZooCache(instance);
 
 Review comment:
   I like that, thanks.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163631700
 
 

 ##
 File path: core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java
 ##
 @@ -129,12 +86,26 @@ public static String getTableName(Instance instance, 
String tableId) throws Tabl
 return tableName;
   }
 
-  public static SortedMap getNameToIdMap(Instance instance) {
-return getMap(instance, true);
+  public static Map getNameToIdMap(Instance instance) {
+TableMap map = tableMapCache;
+// check if map needs to be updated
+if (map == null || tableMapTimestamp.longValue() + 
TABLE_MAP_CACHE_EXPIRATION <= System.nanoTime()) {
+  tableMapCache = new TableMap(instance);
+  tableMapTimestamp.set(System.nanoTime());
+  map = tableMapCache;
+}
+return map.getNameToIdMap();
   }
 
-  public static SortedMap getIdToNameMap(Instance instance) {
-return getMap(instance, false);
+  public static Map getIdToNameMap(Instance instance) {
+TableMap map = tableMapCache;
+// check if map needs to be updated
+if (map == null || tableMapTimestamp.longValue() + 
TABLE_MAP_CACHE_EXPIRATION <= System.nanoTime()) {
 
 Review comment:
   I was thinking of a new static method in Tables class


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] milleruntime commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

milleruntime commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163624188
 
 

 ##
 File path: core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java
 ##
 @@ -147,6 +118,7 @@ public static void clearCache(Instance instance) {
 cacheResetCount.incrementAndGet();
 getZooCache(instance).clear(ZooUtil.getRoot(instance) + Constants.ZTABLES);
 getZooCache(instance).clear(ZooUtil.getRoot(instance) + 
Constants.ZNAMESPACES);
+tableMapCache = null;
 
 Review comment:
   Good catch, thank you!


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] milleruntime commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

milleruntime commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163624052
 
 

 ##
 File path: core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java
 ##
 @@ -129,12 +86,26 @@ public static String getTableName(Instance instance, 
String tableId) throws Tabl
 return tableName;
   }
 
-  public static SortedMap getNameToIdMap(Instance instance) {
-return getMap(instance, true);
+  public static Map getNameToIdMap(Instance instance) {
+TableMap map = tableMapCache;
+// check if map needs to be updated
+if (map == null || tableMapTimestamp.longValue() + 
TABLE_MAP_CACHE_EXPIRATION <= System.nanoTime()) {
+  tableMapCache = new TableMap(instance);
+  tableMapTimestamp.set(System.nanoTime());
+  map = tableMapCache;
+}
+return map.getNameToIdMap();
   }
 
-  public static SortedMap getIdToNameMap(Instance instance) {
-return getMap(instance, false);
+  public static Map getIdToNameMap(Instance instance) {
+TableMap map = tableMapCache;
+// check if map needs to be updated
+if (map == null || tableMapTimestamp.longValue() + 
TABLE_MAP_CACHE_EXPIRATION <= System.nanoTime()) {
 
 Review comment:
   I had thought about this as well but wasn't sure where to put it in 
TableMap.  You have this replacing the constructor?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] milleruntime commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

milleruntime commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163623382
 
 

 ##
 File path: 
core/src/main/java/org/apache/accumulo/core/client/impl/TableMap.java
 ##
 @@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.core.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.client.impl.Tables.getZooCache;
+import static org.apache.accumulo.core.client.impl.Tables.qualified;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.client.Instance;
+import org.apache.accumulo.core.client.NamespaceNotFoundException;
+import org.apache.accumulo.core.zookeeper.ZooUtil;
+import org.apache.accumulo.fate.zookeeper.ZooCache;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.collect.ImmutableMap;
+
+/**
+ * Used for thread safe caching of table ID maps
+ */
+public class TableMap {
+  private static final Logger log = LoggerFactory.getLogger(TableMap.class);
+
+  private Map tableNameToIdMap;
+  private Map tableIdtoNameMap;
+
+  public TableMap(Instance instance) {
+generateMaps(instance);
 
 Review comment:
   Good call.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Accumulo-Pull-Requests - Build # 982 - Failure

[Apache Jenkins Server]

The Apache Jenkins build system has built Accumulo-Pull-Requests (build #982)

Status: Failure

Check console output at 
https://builds.apache.org/job/Accumulo-Pull-Requests/982/ to view the results.

Accumulo-Pull-Requests - Build # 981 - Fixed

[Apache Jenkins Server]

The Apache Jenkins build system has built Accumulo-Pull-Requests (build #981)

Status: Fixed

Check console output at 
https://builds.apache.org/job/Accumulo-Pull-Requests/981/ to view the results.

[GitHub] keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163593602
 
 

 ##
 File path: core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java
 ##
 @@ -37,66 +35,25 @@
 import org.apache.accumulo.core.zookeeper.ZooUtil;
 import org.apache.accumulo.fate.zookeeper.ZooCache;
 import org.apache.accumulo.fate.zookeeper.ZooCacheFactory;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 public class Tables {
-  private static final Logger log = LoggerFactory.getLogger(Tables.class);
 
   public static final String VALID_NAME_REGEX = "^(\\w+\\.)?(\\w+)$";
+  public static final Long TABLE_MAP_CACHE_EXPIRATION = 
TimeUnit.SECONDS.toNanos(1L);
+  private static final AtomicLong tableMapTimestamp = new 
AtomicLong(System.nanoTime());
 
   private static final SecurityPermission TABLES_PERMISSION = new 
SecurityPermission("tablesPermission");
   private static final AtomicLong cacheResetCount = new AtomicLong(0);
+  private static volatile TableMap tableMapCache;
 
-  private static ZooCache getZooCache(Instance instance) {
+  public static ZooCache getZooCache(Instance instance) {
 
 Review comment:
   If the ZooCache is passed to the TableMap constructor, then this could go 
back to private.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163592859
 
 

 ##
 File path: 
core/src/main/java/org/apache/accumulo/core/client/impl/TableMap.java
 ##
 @@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.core.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.client.impl.Tables.getZooCache;
+import static org.apache.accumulo.core.client.impl.Tables.qualified;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.client.Instance;
+import org.apache.accumulo.core.client.NamespaceNotFoundException;
+import org.apache.accumulo.core.zookeeper.ZooUtil;
+import org.apache.accumulo.fate.zookeeper.ZooCache;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.collect.ImmutableMap;
+
+/**
+ * Used for thread safe caching of table ID maps
+ */
+public class TableMap {
+  private static final Logger log = LoggerFactory.getLogger(TableMap.class);
+
+  private Map tableNameToIdMap;
+  private Map tableIdtoNameMap;
+
+  public TableMap(Instance instance) {
+generateMaps(instance);
+  }
+
+  private void generateMaps(Instance instance) {
+ZooCache zc = getZooCache(instance);
 
 Review comment:
   An alternative to calling getZooCache here, is to pass the ZooCache into the 
constructor.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163586655
 
 

 ##
 File path: core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java
 ##
 @@ -37,66 +35,25 @@
 import org.apache.accumulo.core.zookeeper.ZooUtil;
 import org.apache.accumulo.fate.zookeeper.ZooCache;
 import org.apache.accumulo.fate.zookeeper.ZooCacheFactory;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 public class Tables {
-  private static final Logger log = LoggerFactory.getLogger(Tables.class);
 
   public static final String VALID_NAME_REGEX = "^(\\w+\\.)?(\\w+)$";
+  public static final Long TABLE_MAP_CACHE_EXPIRATION = 
TimeUnit.SECONDS.toNanos(1L);
+  private static final AtomicLong tableMapTimestamp = new 
AtomicLong(System.nanoTime());
 
   private static final SecurityPermission TABLES_PERMISSION = new 
SecurityPermission("tablesPermission");
   private static final AtomicLong cacheResetCount = new AtomicLong(0);
+  private static volatile TableMap tableMapCache;
 
-  private static ZooCache getZooCache(Instance instance) {
+  public static ZooCache getZooCache(Instance instance) {
 
 Review comment:
   Could make this package private


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163591990
 
 

 ##
 File path: 
core/src/main/java/org/apache/accumulo/core/client/impl/TableMap.java
 ##
 @@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.core.client.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.client.impl.Tables.getZooCache;
+import static org.apache.accumulo.core.client.impl.Tables.qualified;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.client.Instance;
+import org.apache.accumulo.core.client.NamespaceNotFoundException;
+import org.apache.accumulo.core.zookeeper.ZooUtil;
+import org.apache.accumulo.fate.zookeeper.ZooCache;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.collect.ImmutableMap;
+
+/**
+ * Used for thread safe caching of table ID maps
+ */
+public class TableMap {
+  private static final Logger log = LoggerFactory.getLogger(TableMap.class);
+
+  private Map tableNameToIdMap;
+  private Map tableIdtoNameMap;
+
+  public TableMap(Instance instance) {
+generateMaps(instance);
 
 Review comment:
   If this code is inlined in the constructor then the two maps could be made 
final.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163591644
 
 

 ##
 File path: core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java
 ##
 @@ -129,12 +86,26 @@ public static String getTableName(Instance instance, 
String tableId) throws Tabl
 return tableName;
   }
 
-  public static SortedMap getNameToIdMap(Instance instance) {
-return getMap(instance, true);
+  public static Map getNameToIdMap(Instance instance) {
+TableMap map = tableMapCache;
+// check if map needs to be updated
+if (map == null || tableMapTimestamp.longValue() + 
TABLE_MAP_CACHE_EXPIRATION <= System.nanoTime()) {
+  tableMapCache = new TableMap(instance);
+  tableMapTimestamp.set(System.nanoTime());
+  map = tableMapCache;
+}
+return map.getNameToIdMap();
   }
 
-  public static SortedMap getIdToNameMap(Instance instance) {
-return getMap(instance, false);
+  public static Map getIdToNameMap(Instance instance) {
+TableMap map = tableMapCache;
+// check if map needs to be updated
+if (map == null || tableMapTimestamp.longValue() + 
TABLE_MAP_CACHE_EXPIRATION <= System.nanoTime()) {
 
 Review comment:
   Could make the TableMap class have a final long creationTime field instead 
of having the mutable tableMapTimestamp.  Then the CPU may be able to avoid two 
trips to main memory.  Also I think keeping it all together and immutable makes 
it easier to reason about.
   
   Could also do a double check to avoid multiple threads regenerating and put 
all of this in a single function.
   
   ```java
   private static TableMap getTableMap() {
 TableMap map = tableMapCache;
 if (map == null || map.getCreationTime() + TABLE_MAP_CACHE_EXPIRATION <= 
System.nanoTime()) {
   synchronized(Tables.class) {
  map = tableMapCache;
  if (map == null || map.getCreationTime() + TABLE_MAP_CACHE_EXPIRATION 
<= System.nanoTime()) {
map = tableMapCache = new TableMap(instance);  //this could set 
creation time, should do that last in constructor
  } 
   }
 }
 return map;
   }
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

keith-turner commented on a change in pull request #364: ACCUMULO-4778 Initial 
feedback for table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364#discussion_r163588542
 
 

 ##
 File path: core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java
 ##
 @@ -147,6 +118,7 @@ public static void clearCache(Instance instance) {
 cacheResetCount.incrementAndGet();
 getZooCache(instance).clear(ZooUtil.getRoot(instance) + Constants.ZTABLES);
 getZooCache(instance).clear(ZooUtil.getRoot(instance) + 
Constants.ZNAMESPACES);
+tableMapCache = null;
 
 Review comment:
   There is also a clearCacheByPath method where this should be set to null


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[jira] [Updated] (ACCUMULO-4778) Resolving table name to table id is expensive

[Michael Miller (JIRA)]

 [ 
https://issues.apache.org/jira/browse/ACCUMULO-4778?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Michael Miller updated ACCUMULO-4778:
-
Fix Version/s: 1.9.0
   1.7.4

> Resolving table name to table id is expensive
> -
>
> Key: ACCUMULO-4778
> URL: https://issues.apache.org/jira/browse/ACCUMULO-4778
> Project: Accumulo
>  Issue Type: Bug
>Affects Versions: 1.7.3, 1.8.1
>Reporter: Keith Turner
>Assignee: Michael Miller
>Priority: Major
>  Labels: pull-request-available
> Fix For: 1.7.4, 1.9.0, 2.0.0
>
>  Time Spent: 10m
>  Remaining Estimate: 0h
>
> I was running a Fluo test application and profiling the tablet server and 
> Fluo worker.  The Fluo worker does lots small scans against Accumulo.   
> Resolving table names to ids (which is done for each scan) was expensive 
> enough to make a significant showing in the profiling data.
> I looked that the 1.8 code and it does the following to resolve a table name :
>  * reads over all cached table ids in zookeeper putting them in a treemap
>  * does a lookup in the treemap 
> Ideally the client code would keep a cache of name to id mappings and 
> invalidate them when something changes in zookeeper.  The data in zookeeper 
> is stored by id, so it does need to be inverted to lookup by name.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Updated] (ACCUMULO-4778) Resolving table name to table id is expensive

[ASF GitHub Bot (JIRA)]

 [ 
https://issues.apache.org/jira/browse/ACCUMULO-4778?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

ASF GitHub Bot updated ACCUMULO-4778:
-
Labels: pull-request-available  (was: )

> Resolving table name to table id is expensive
> -
>
> Key: ACCUMULO-4778
> URL: https://issues.apache.org/jira/browse/ACCUMULO-4778
> Project: Accumulo
>  Issue Type: Bug
>Affects Versions: 1.7.3, 1.8.1
>Reporter: Keith Turner
>Assignee: Michael Miller
>Priority: Major
>  Labels: pull-request-available
> Fix For: 2.0.0
>
>
> I was running a Fluo test application and profiling the tablet server and 
> Fluo worker.  The Fluo worker does lots small scans against Accumulo.   
> Resolving table names to ids (which is done for each scan) was expensive 
> enough to make a significant showing in the profiling data.
> I looked that the 1.8 code and it does the following to resolve a table name :
>  * reads over all cached table ids in zookeeper putting them in a treemap
>  * does a lookup in the treemap 
> Ideally the client code would keep a cache of name to id mappings and 
> invalidate them when something changes in zookeeper.  The data in zookeeper 
> is stored by id, so it does need to be inverted to lookup by name.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[GitHub] milleruntime opened a new pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache

[GitBox]

milleruntime opened a new pull request #364: ACCUMULO-4778 Initial feedback for 
table name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364
 
 
   Looking for some initial feedback.  Running Sunny tests now but haven't 
attempted to measure performance yet.
   
   Class generates table id to name mapping every second.  Previously, a 
mapping (of all table ids) was generated every time a single table id or name 
look up was called.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Accumulo-Pull-Requests - Build # 980 - Failure

[Apache Jenkins Server]

The Apache Jenkins build system has built Accumulo-Pull-Requests (build #980)

Status: Failure

Check console output at 
https://builds.apache.org/job/Accumulo-Pull-Requests/980/ to view the results.