From e4ed9106802b553ae89f148c164b90a873c35271 Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Fri, 11 Oct 2019 15:01:38 -0400 Subject: [PATCH 01/11] Fixes #1220 - Only watch and cache values actually in ZooKeeper and properly remove all table config watchers on table delete --- .../org/apache/accumulo/core/Constants.java | 1 + .../accumulo/fate/zookeeper/ZooCache.java | 56 +++++++++++++++++-- .../accumulo/server/tables/TableManager.java | 11 ++++ 3 files changed, 64 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/Constants.java b/core/src/main/java/org/apache/accumulo/core/Constants.java index 7d232e1ad6d..bc52e94e84b 100644 --- a/core/src/main/java/org/apache/accumulo/core/Constants.java +++ b/core/src/main/java/org/apache/accumulo/core/Constants.java @@ -118,4 +118,5 @@ public class Constants { public static final String HDFS_TABLES_DIR = "/tables"; public static final int DEFAULT_VISIBILITY_CACHE_SIZE = 1000; + } diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index 4f521d86474..c80f92b99ff 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -63,6 +63,12 @@ public class ZooCache { private volatile boolean closed = false; + public static final String[] extraConfigs = {"table.split.endrow.size.max", + "table.groups.enabled", "table.split.threshold", "table.classpath.context", + "table.compaction.minor.logs.threshold", "table.balancer", "table.replication", + "table.majc.compaction.strategy", "tserver.memory.maps.native.enabled", "tserver.dir.memdump", + "tserver.walog.max.referenced"}; + public static class ZcStat { private long ephemeralOwner; private long mzxid; @@ -402,18 +408,25 @@ public byte[] run() throws KeeperException, InterruptedException { * a special case that looks for Code.NONODE in the KeeperException, then non-existence can * not be cached. */ + cacheWriteLock.lock(); try { + final ZooKeeper zooKeeper = getZooKeeper(); - Stat stat = zooKeeper.exists(zPath, watcher); + byte[] data = null; + + boolean watched = isWatched(zooKeeper, zPath); + + Stat stat = zooKeeper.exists(zPath, watched ? watcher : null); + if (stat == null) { if (log.isTraceEnabled()) { log.trace("zookeeper did not contain {}", zPath); } } else { try { - data = zooKeeper.getData(zPath, watcher, stat); + data = zooKeeper.getData(zPath, watched ? watcher : null, stat); zstat = new ZcStat(stat); } catch (KeeperException.BadVersionException | KeeperException.NoNodeException e1) { throw new ConcurrentModificationException(); @@ -423,8 +436,10 @@ public byte[] run() throws KeeperException, InterruptedException { (data == null ? null : new String(data, UTF_8))); } } - put(zPath, data, zstat); - copyStats(status, zstat); + if (watched) { + put(zPath, data, zstat); + copyStats(status, zstat); + } return data; } finally { cacheWriteLock.unlock(); @@ -435,6 +450,39 @@ public byte[] run() throws KeeperException, InterruptedException { return zr.retry(); } + /* + * Some of the table configuration parameters are possibly not in ZooKeeper yet and even though + * you are able you should not put a watcher on them to observe the NodeCreated event. If you do + * that and the ZNode for that configuration is never created, you will have no way to delete the + * watcher you created for it in Zookeeper. Unremovable watchers that are created for for table + * configs that never actually have a Znode is causing memory leaks in Accumulo instances that + * create and delete tables at a high rate. + */ + + private boolean isWatched(ZooKeeper zooKeeper, String zPath) + throws KeeperException, InterruptedException { + + boolean watched = true; + + for (String possiblyWatchedConfig : extraConfigs) { + Stat stat; + if (zPath.endsWith(possiblyWatchedConfig)) { + try { + stat = zooKeeper.exists(zPath, false); + } catch (KeeperException.BadVersionException | KeeperException.NoNodeException e1) { + throw new ConcurrentModificationException(); + } + if (stat != null) { + watched = true; + } else { + watched = false; + } + break; + } + } + return watched; + } + /** * Helper method to copy stats from the cached stat into userStat * diff --git a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java index 1f96f9c1cbd..557007174d8 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java +++ b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Set; @@ -45,6 +46,7 @@ import org.apache.zookeeper.WatchedEvent; import org.apache.zookeeper.Watcher; import org.apache.zookeeper.Watcher.Event.EventType; +import org.apache.zookeeper.data.Stat; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -250,6 +252,15 @@ public void removeTable(TableId tableId) throws KeeperException, InterruptedExce tableStateCache.remove(tableId); zoo.recursiveDelete(zkRoot + Constants.ZTABLES + "/" + tableId + Constants.ZTABLE_STATE, NodeMissingPolicy.SKIP); + String tableConfigPath = zkRoot + Constants.ZTABLES + "/" + tableId + Constants.ZTABLE_CONF; + List tableConfigs = zoo.getZooKeeper().getChildren(tableConfigPath, false); + for (String tableConfig : tableConfigs) { + log.trace("Deleting the table config " + tableConfigPath + "/" + tableConfig); + org.apache.zookeeper.data.Stat stat = + zoo.getZooKeeper().exists(tableConfigPath + "/" + tableConfig, false); + zoo.getZooKeeper().delete(tableConfigPath + "/" + tableConfig, stat.getVersion()); + } + zoo.recursiveDelete(zkRoot + Constants.ZTABLES + "/" + tableId, NodeMissingPolicy.SKIP); } } From 743cb489f3507043e0bb4ffef42364dbbf48f0db Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Tue, 12 Nov 2019 14:17:22 -0500 Subject: [PATCH 02/11] Removed unneeded call to the zookeeper.delete --- .../java/org/apache/accumulo/server/tables/TableManager.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java index 557007174d8..f0dc28ec5e2 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java +++ b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java @@ -258,7 +258,6 @@ public void removeTable(TableId tableId) throws KeeperException, InterruptedExce log.trace("Deleting the table config " + tableConfigPath + "/" + tableConfig); org.apache.zookeeper.data.Stat stat = zoo.getZooKeeper().exists(tableConfigPath + "/" + tableConfig, false); - zoo.getZooKeeper().delete(tableConfigPath + "/" + tableConfig, stat.getVersion()); } zoo.recursiveDelete(zkRoot + Constants.ZTABLES + "/" + tableId, NodeMissingPolicy.SKIP); From a828adfac412eb0414a97251a894c5405877c435 Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Wed, 13 Nov 2019 17:22:13 -0500 Subject: [PATCH 03/11] Got rid of the ugly iteration through possible table configs - found optimal solution --- .../accumulo/fate/zookeeper/ZooCache.java | 37 +++++++++---------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index c80f92b99ff..09ca28f4030 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -57,18 +57,13 @@ public class ZooCache { private final HashMap cache; private final HashMap statCache; private final HashMap> childrenCache; + private final HashMap znodeExists = new HashMap<>(); private final ZooReader zReader; private final SecureRandom secureRandom = new SecureRandom(); private volatile boolean closed = false; - public static final String[] extraConfigs = {"table.split.endrow.size.max", - "table.groups.enabled", "table.split.threshold", "table.classpath.context", - "table.compaction.minor.logs.threshold", "table.balancer", "table.replication", - "table.majc.compaction.strategy", "tserver.memory.maps.native.enabled", "tserver.dir.memdump", - "tserver.walog.max.referenced"}; - public static class ZcStat { private long ephemeralOwner; private long mzxid; @@ -464,22 +459,25 @@ private boolean isWatched(ZooKeeper zooKeeper, String zPath) boolean watched = true; - for (String possiblyWatchedConfig : extraConfigs) { + if (znodeExists.containsKey(zPath)) + return watched; + + if (zPath.contains("tserver.") || zPath.contains("table.")) { Stat stat; - if (zPath.endsWith(possiblyWatchedConfig)) { - try { - stat = zooKeeper.exists(zPath, false); - } catch (KeeperException.BadVersionException | KeeperException.NoNodeException e1) { - throw new ConcurrentModificationException(); - } - if (stat != null) { - watched = true; - } else { - watched = false; - } - break; + try { + stat = zooKeeper.exists(zPath, false); + } catch (KeeperException.BadVersionException | KeeperException.NoNodeException e1) { + throw new ConcurrentModificationException(); + } + + if (stat != null) { + watched = true; + znodeExists.put(zPath, true); + } else { + watched = false; } } + return watched; } @@ -516,6 +514,7 @@ private void remove(String zPath) { cache.remove(zPath); childrenCache.remove(zPath); statCache.remove(zPath); + znodeExists.remove(zPath); immutableCache = new ImmutableCacheCopies(++updateCount, cache, statCache, childrenCache); } finally { From e971c0bb5a2fc77813f0c85dfa0ce69efec970c6 Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Thu, 14 Nov 2019 10:25:48 -0500 Subject: [PATCH 04/11] Fixes #1220 - Only watch and cache values actually in ZooKeeper and properly remove all table config watchers on table delete --- .../org/apache/accumulo/fate/zookeeper/ZooCache.java | 4 +++- .../apache/accumulo/server/tables/TableManager.java | 12 +++--------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index 09ca28f4030..824d889f84f 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -472,7 +472,9 @@ private boolean isWatched(ZooKeeper zooKeeper, String zPath) if (stat != null) { watched = true; - znodeExists.put(zPath, true); + synchronized (znodeExists) { + znodeExists.put(zPath, true); + } } else { watched = false; } diff --git a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java index f0dc28ec5e2..5bed9f12d26 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java +++ b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java @@ -21,7 +21,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Set; @@ -46,7 +45,6 @@ import org.apache.zookeeper.WatchedEvent; import org.apache.zookeeper.Watcher; import org.apache.zookeeper.Watcher.Event.EventType; -import org.apache.zookeeper.data.Stat; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -253,13 +251,9 @@ public void removeTable(TableId tableId) throws KeeperException, InterruptedExce zoo.recursiveDelete(zkRoot + Constants.ZTABLES + "/" + tableId + Constants.ZTABLE_STATE, NodeMissingPolicy.SKIP); String tableConfigPath = zkRoot + Constants.ZTABLES + "/" + tableId + Constants.ZTABLE_CONF; - List tableConfigs = zoo.getZooKeeper().getChildren(tableConfigPath, false); - for (String tableConfig : tableConfigs) { - log.trace("Deleting the table config " + tableConfigPath + "/" + tableConfig); - org.apache.zookeeper.data.Stat stat = - zoo.getZooKeeper().exists(tableConfigPath + "/" + tableConfig, false); - } - + // The line below removes all the watches on the table configuration items from the Zookeeper + // server + zoo.getZooKeeper().getChildren(tableConfigPath, false); zoo.recursiveDelete(zkRoot + Constants.ZTABLES + "/" + tableId, NodeMissingPolicy.SKIP); } } From 4f74ca3a13d5c39537e5c99548b215fc8f3187b0 Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Thu, 21 Nov 2019 10:52:59 -0500 Subject: [PATCH 05/11] Refreshing table configs when the NodeChildrenChange occcurs on conf directories --- .../apache/accumulo/fate/zookeeper/ZooCache.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index 824d889f84f..f2b8df93db5 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -158,6 +158,20 @@ public void process(WatchedEvent event) { switch (event.getType()) { case NodeDataChanged: case NodeChildrenChanged: + if (event.getPath().endsWith("conf") + && event.getType().equals(Event.EventType.NodeChildrenChanged)) { + try { + clear(event.getPath()); + getZooKeeper().exists(event.getPath(), watcher); + if (log.isTraceEnabled()) { + log.trace("NodeChildrenChanged: resetting watcher for " + event.getPath()); + if (externalWatcher != null) + log.trace("external watcher with process this zpath also " + event.getPath()); + } + } catch (KeeperException | InterruptedException e) { + log.error("could not reset watcher on parent node: " + event.getPath()); + } + } case NodeCreated: case NodeDeleted: remove(event.getPath()); From 71cc21d8c9eab3386d964732590c8cd22e2aea44 Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Tue, 26 Nov 2019 12:27:05 -0500 Subject: [PATCH 06/11] Moved a case around in a switch statement --- .../java/org/apache/accumulo/fate/zookeeper/ZooCache.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index f2b8df93db5..cd1f58fcf27 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -156,10 +156,8 @@ public void process(WatchedEvent event) { } switch (event.getType()) { - case NodeDataChanged: case NodeChildrenChanged: - if (event.getPath().endsWith("conf") - && event.getType().equals(Event.EventType.NodeChildrenChanged)) { + if (event.getPath().endsWith("conf")) { try { clear(event.getPath()); getZooKeeper().exists(event.getPath(), watcher); @@ -172,6 +170,7 @@ public void process(WatchedEvent event) { log.error("could not reset watcher on parent node: " + event.getPath()); } } + case NodeDataChanged: case NodeCreated: case NodeDeleted: remove(event.getPath()); From 0f311d1371eb3c887971a42fde553cb48f265818 Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Wed, 27 Nov 2019 08:11:51 -0500 Subject: [PATCH 07/11] If watched is false return from get(zPath) right away --- .../accumulo/fate/zookeeper/ZooCache.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index cd1f58fcf27..dcc34e9af99 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -424,9 +424,14 @@ public byte[] run() throws KeeperException, InterruptedException { byte[] data = null; + //watched will be false if it is a table config that is not + //actually a node inside Zookeeper boolean watched = isWatched(zooKeeper, zPath); - Stat stat = zooKeeper.exists(zPath, watched ? watcher : null); + if (!watched) + return data; + + Stat stat = zooKeeper.exists(zPath, watcher); if (stat == null) { if (log.isTraceEnabled()) { @@ -434,7 +439,7 @@ public byte[] run() throws KeeperException, InterruptedException { } } else { try { - data = zooKeeper.getData(zPath, watched ? watcher : null, stat); + data = zooKeeper.getData(zPath, watcher, stat); zstat = new ZcStat(stat); } catch (KeeperException.BadVersionException | KeeperException.NoNodeException e1) { throw new ConcurrentModificationException(); @@ -444,10 +449,10 @@ public byte[] run() throws KeeperException, InterruptedException { (data == null ? null : new String(data, UTF_8))); } } - if (watched) { - put(zPath, data, zstat); - copyStats(status, zstat); - } + + put(zPath, data, zstat); + copyStats(status, zstat); + return data; } finally { cacheWriteLock.unlock(); From f32d087b186aca310df49066c79bf39b422408f9 Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Wed, 27 Nov 2019 09:25:59 -0500 Subject: [PATCH 08/11] Fixes #1220 - Removes unused Zookeeper watches when table is deleted --- .../org/apache/accumulo/core/Constants.java | 1 - .../accumulo/fate/zookeeper/ZooCache.java | 37 +++++++++++++------ 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/Constants.java b/core/src/main/java/org/apache/accumulo/core/Constants.java index bc52e94e84b..7d232e1ad6d 100644 --- a/core/src/main/java/org/apache/accumulo/core/Constants.java +++ b/core/src/main/java/org/apache/accumulo/core/Constants.java @@ -118,5 +118,4 @@ public class Constants { public static final String HDFS_TABLES_DIR = "/tables"; public static final int DEFAULT_VISIBILITY_CACHE_SIZE = 1000; - } diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index dcc34e9af99..0d95923a9db 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -28,7 +28,10 @@ import java.util.concurrent.locks.LockSupport; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.apache.accumulo.core.Constants; import org.apache.zookeeper.KeeperException; import org.apache.zookeeper.KeeperException.Code; import org.apache.zookeeper.WatchedEvent; @@ -47,6 +50,13 @@ public class ZooCache { private static final Logger log = LoggerFactory.getLogger(ZooCache.class); + public final static Pattern TABLE_CONFIG_DIR_PATTERN = Pattern.compile( + "(/accumulo/[0-9a-z-]+)(" + Constants.ZTABLES + ")(/.*)(" + Constants.ZTABLE_CONF + ")"); + + public final static Pattern TABLE_SETTING_CONFIG_PATTERN = + Pattern.compile("(/accumulo/[0-9a-z-]+)(" + Constants.ZTABLES + ")(/.*)(" + + Constants.ZTABLE_CONF + ")/(table.*|tserver.*)"); + private final ZCacheWatcher watcher = new ZCacheWatcher(); private final Watcher externalWatcher; @@ -158,16 +168,17 @@ public void process(WatchedEvent event) { switch (event.getType()) { case NodeChildrenChanged: if (event.getPath().endsWith("conf")) { - try { - clear(event.getPath()); - getZooKeeper().exists(event.getPath(), watcher); - if (log.isTraceEnabled()) { - log.trace("NodeChildrenChanged: resetting watcher for " + event.getPath()); - if (externalWatcher != null) - log.trace("external watcher with process this zpath also " + event.getPath()); + Matcher confDirMatcher = TABLE_CONFIG_DIR_PATTERN.matcher(event.getPath()); + if (confDirMatcher.matches()) { + try { + clear(event.getPath()); + getZooKeeper().exists(event.getPath(), watcher); + if (log.isTraceEnabled()) { + log.trace("NodeChildrenChanged: resetting watcher for " + event.getPath()); + } + } catch (KeeperException | InterruptedException e) { + log.error("could not reset watcher on parent node: " + event.getPath()); } - } catch (KeeperException | InterruptedException e) { - log.error("could not reset watcher on parent node: " + event.getPath()); } } case NodeDataChanged: @@ -424,8 +435,8 @@ public byte[] run() throws KeeperException, InterruptedException { byte[] data = null; - //watched will be false if it is a table config that is not - //actually a node inside Zookeeper + // watched will be false if it is a table config that is not + // actually a node inside Zookeeper boolean watched = isWatched(zooKeeper, zPath); if (!watched) @@ -480,7 +491,9 @@ private boolean isWatched(ZooKeeper zooKeeper, String zPath) if (znodeExists.containsKey(zPath)) return watched; - if (zPath.contains("tserver.") || zPath.contains("table.")) { + Matcher configMatcher = TABLE_SETTING_CONFIG_PATTERN.matcher(zPath); + + if (configMatcher.matches()) { Stat stat; try { stat = zooKeeper.exists(zPath, false); From ab26c9c1e5ee2f2945c41132551e0c42af43bb70 Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Mon, 2 Dec 2019 11:05:24 -0500 Subject: [PATCH 09/11] Fixes #1220 - Removes unused Zookeeper watches when table is deleted --- .../apache/accumulo/fate/zookeeper/ZooCache.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index 0d95923a9db..0f174ce28fe 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -22,6 +22,7 @@ import java.util.Collections; import java.util.ConcurrentModificationException; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.concurrent.locks.Lock; @@ -67,7 +68,7 @@ public class ZooCache { private final HashMap cache; private final HashMap statCache; private final HashMap> childrenCache; - private final HashMap znodeExists = new HashMap<>(); + private final HashSet znodeExists = new HashSet<>(); private final ZooReader zReader; private final SecureRandom secureRandom = new SecureRandom(); @@ -173,9 +174,9 @@ public void process(WatchedEvent event) { try { clear(event.getPath()); getZooKeeper().exists(event.getPath(), watcher); - if (log.isTraceEnabled()) { - log.trace("NodeChildrenChanged: resetting watcher for " + event.getPath()); - } + if (log.isTraceEnabled()) { + log.trace("NodeChildrenChanged: resetting watcher for " + event.getPath()); + } } catch (KeeperException | InterruptedException e) { log.error("could not reset watcher on parent node: " + event.getPath()); } @@ -488,7 +489,7 @@ private boolean isWatched(ZooKeeper zooKeeper, String zPath) boolean watched = true; - if (znodeExists.containsKey(zPath)) + if (znodeExists.contains(zPath)) return watched; Matcher configMatcher = TABLE_SETTING_CONFIG_PATTERN.matcher(zPath); @@ -504,7 +505,7 @@ private boolean isWatched(ZooKeeper zooKeeper, String zPath) if (stat != null) { watched = true; synchronized (znodeExists) { - znodeExists.put(zPath, true); + znodeExists.add(zPath); } } else { watched = false; From 3f046bdbddadd9c472dd5128971632894d71d22a Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Tue, 3 Dec 2019 16:05:23 -0500 Subject: [PATCH 10/11] Fixes #1220 - Removes unused Zookeeper watches when table is deleted --- .../accumulo/fate/zookeeper/ZooCache.java | 2 +- .../accumulo/server/tables/TableManager.java | 30 +++++++++---------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index 0f174ce28fe..7eeede8e63e 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -499,7 +499,7 @@ private boolean isWatched(ZooKeeper zooKeeper, String zPath) try { stat = zooKeeper.exists(zPath, false); } catch (KeeperException.BadVersionException | KeeperException.NoNodeException e1) { - throw new ConcurrentModificationException(); + throw e1; } if (stat != null) { diff --git a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java index 5bed9f12d26..303e558e24f 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java +++ b/server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java @@ -1,18 +1,20 @@ /* - * 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 + * 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 + * 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. + * 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.server.tables; @@ -250,10 +252,6 @@ public void removeTable(TableId tableId) throws KeeperException, InterruptedExce tableStateCache.remove(tableId); zoo.recursiveDelete(zkRoot + Constants.ZTABLES + "/" + tableId + Constants.ZTABLE_STATE, NodeMissingPolicy.SKIP); - String tableConfigPath = zkRoot + Constants.ZTABLES + "/" + tableId + Constants.ZTABLE_CONF; - // The line below removes all the watches on the table configuration items from the Zookeeper - // server - zoo.getZooKeeper().getChildren(tableConfigPath, false); zoo.recursiveDelete(zkRoot + Constants.ZTABLES + "/" + tableId, NodeMissingPolicy.SKIP); } } From 841e0bdc34f0299d35bc74dd17e4eb22ddc9265b Mon Sep 17 00:00:00 2001 From: Jeffrey Zeiberg Date: Wed, 4 Dec 2019 13:38:24 -0500 Subject: [PATCH 11/11] Remove isTraceEnabled and changed logging level to INFO in one statement --- .../org/apache/accumulo/fate/zookeeper/ZooCache.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java index 7eeede8e63e..760e39bd672 100644 --- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java +++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java @@ -172,13 +172,14 @@ public void process(WatchedEvent event) { Matcher confDirMatcher = TABLE_CONFIG_DIR_PATTERN.matcher(event.getPath()); if (confDirMatcher.matches()) { try { + // If a table config parameter without a watch on it is deleted + // this is how it gets cleared from ZooCache clear(event.getPath()); getZooKeeper().exists(event.getPath(), watcher); - if (log.isTraceEnabled()) { - log.trace("NodeChildrenChanged: resetting watcher for " + event.getPath()); - } + log.info("NodeChildrenChanged: resetting watcher for " + event.getPath()); + } catch (KeeperException | InterruptedException e) { - log.error("could not reset watcher on parent node: " + event.getPath()); + log.error("Could not reset watcher on parent node: " + event.getPath()); } } }