You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ctubbsii (via GitHub)" <gi...@apache.org> on 2025/03/19 23:22:39 UTC
[PR] Move static location cache methods to ClientContext [accumulo]
ctubbsii opened a new pull request, #5421:
URL: https://github.com/apache/accumulo/pull/5421
Move static synchronized methods from ClientTabletCache into the ClientContext, with synchronization only on the collection for reading and clearing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2014666423
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1098,43 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ return tabletLocationCache.get(DataLevel.of(tableId)).computeIfAbsent(tableId,
+ (TableId key) -> {
+ var lockChecker = getTServerLockChecker();
+ if (AccumuloTable.ROOT.tableId().equals(tableId)) {
+ return new RootClientTabletCache(lockChecker);
+ }
+ var mlo = new MetadataCachedTabletObtainer();
+ if (AccumuloTable.METADATA.tableId().equals(tableId)) {
+ return new ClientTabletCacheImpl(AccumuloTable.METADATA.tableId(),
+ getTabletLocationCache(AccumuloTable.ROOT.tableId()), mlo, lockChecker);
+ } else {
+ return new ClientTabletCacheImpl(tableId,
+ getTabletLocationCache(AccumuloTable.METADATA.tableId()), mlo, lockChecker);
+ }
+ });
+ }
+
+ /**
+ * Clear the currently cached tablet locations. The use of ConcurrentHashMap ensures this is
+ * thread-safe. However, since the ConcurrentHashMap iterator is weakly consistent, it does not
+ * block new locations from being cached. If new locations are added while this is executing, they
+ * may be immediately invalidated by this code. Multiple calls to this method in different threads
+ * may cause some location caches to be invalidated multiple times. That is okay, because cache
+ * invalidation is idempotent.
+ */
+ public void clearTabletLocationCache() {
+ tabletLocationCache.forEach((dataLevel, map) -> {
+ // use iter.remove() instead of calling clear() on the map, to prevent clearing entries that
+ // may not have been invalidated
+ var iter = map.values().iterator();
+ while (iter.hasNext()) {
+ iter.next().invalidate();
+ iter.remove();
Review Comment:
A lot of code will get a reference to a tablet location cache and hang on to it. That could lead to multiple objects caching data for the same table. This is not a new problem w/ this PR, seems like a preexisting problem with this code as the existing code has this same problem.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -229,6 +233,12 @@ public ClientContext(ClientInfo info, AccumuloConfiguration serverConf,
this.info = info;
this.hadoopConf = info.getHadoopConf();
+ var tabletCache = new HashMap<DataLevel,ConcurrentHashMap<TableId,ClientTabletCache>>();
Review Comment:
Could use an EnumMap here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2006107294
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1091,32 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ synchronized (tabletLocationCache) {
+ return tabletLocationCache.computeIfAbsent(tableId, (TableId key) -> {
+ var mlo = new MetadataCachedTabletObtainer();
+ var lockChecker = getTServerLockChecker();
+ if (AccumuloTable.ROOT.tableId().equals(tableId)) {
+ return new RootClientTabletCache(lockChecker);
+ } else if (AccumuloTable.METADATA.tableId().equals(tableId)) {
+ return new ClientTabletCacheImpl(AccumuloTable.METADATA.tableId(),
+ getTabletLocationCache(AccumuloTable.ROOT.tableId()), mlo, lockChecker);
+ } else {
+ return new ClientTabletCacheImpl(tableId,
+ getTabletLocationCache(AccumuloTable.METADATA.tableId()), mlo, lockChecker);
+ }
Review Comment:
`MetadataCachedTabletObtainer` doesn't look particularly expensive to create but its creation could be delayed until its needed here. Maybe something similar to this:
```suggestion
var lockChecker = getTServerLockChecker();
if (AccumuloTable.ROOT.tableId().equals(tableId)) {
return new RootClientTabletCache(lockChecker);
}
var mlo = new MetadataCachedTabletObtainer();
if (AccumuloTable.METADATA.tableId().equals(tableId)) {
return new ClientTabletCacheImpl(AccumuloTable.METADATA.tableId(),
getTabletLocationCache(AccumuloTable.ROOT.tableId()), mlo, lockChecker);
} else {
return new ClientTabletCacheImpl(tableId,
getTabletLocationCache(AccumuloTable.METADATA.tableId()), mlo, lockChecker);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2014958892
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -229,6 +233,12 @@ public ClientContext(ClientInfo info, AccumuloConfiguration serverConf,
this.info = info;
this.hadoopConf = info.getHadoopConf();
+ var tabletCache = new HashMap<DataLevel,ConcurrentHashMap<TableId,ClientTabletCache>>();
Review Comment:
Done in 1cfabe7ac2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#issuecomment-2753099490
@keith-turner I applied your suggestions from above for the DataLevel... but I think we either need to synchronize on the map for both additions and removals (like my previous implementation), or we need to tolerate that caches can be invalidated more than once (my current implementation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2011159650
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1091,32 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ synchronized (tabletLocationCache) {
Review Comment:
I was wondering about that, because it seemed like we could use any Map the way I wrote it, and it didn't seem like there was much benefit to using ConcurrentHashMap. But I looked at the code and it didn't look like it was doing anything to prevent multiple executions of the function... or at least, I didn't notice anything the first time I looked. I will revisit this again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2006472601
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1091,32 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ synchronized (tabletLocationCache) {
+ return tabletLocationCache.computeIfAbsent(tableId, (TableId key) -> {
+ var mlo = new MetadataCachedTabletObtainer();
+ var lockChecker = getTServerLockChecker();
+ if (AccumuloTable.ROOT.tableId().equals(tableId)) {
+ return new RootClientTabletCache(lockChecker);
+ } else if (AccumuloTable.METADATA.tableId().equals(tableId)) {
+ return new ClientTabletCacheImpl(AccumuloTable.METADATA.tableId(),
+ getTabletLocationCache(AccumuloTable.ROOT.tableId()), mlo, lockChecker);
Review Comment:
Inside computeIfAbset this function will call computeIfAbsent again w/ a different key which you are not supposed to do. If the cache were per datalevel, then this would be safe (like if the top level cache was `Map<DataLevel, <Map<TableId,ClientTabletCache>>`
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1091,32 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ synchronized (tabletLocationCache) {
Review Comment:
Should be able to remove this sync block. The javadoc for ConucrrentHashMap.computeIfAbsent says
```
The entire method invocation is performed atomically, so the function is applied at most once per key.
```
so for each tableId the lambda will run atomically.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2017624551
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1098,43 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ return tabletLocationCache.get(DataLevel.of(tableId)).computeIfAbsent(tableId,
+ (TableId key) -> {
+ var lockChecker = getTServerLockChecker();
+ if (AccumuloTable.ROOT.tableId().equals(tableId)) {
+ return new RootClientTabletCache(lockChecker);
+ }
+ var mlo = new MetadataCachedTabletObtainer();
+ if (AccumuloTable.METADATA.tableId().equals(tableId)) {
+ return new ClientTabletCacheImpl(AccumuloTable.METADATA.tableId(),
+ getTabletLocationCache(AccumuloTable.ROOT.tableId()), mlo, lockChecker);
+ } else {
+ return new ClientTabletCacheImpl(tableId,
+ getTabletLocationCache(AccumuloTable.METADATA.tableId()), mlo, lockChecker);
+ }
+ });
+ }
+
+ /**
+ * Clear the currently cached tablet locations. The use of ConcurrentHashMap ensures this is
+ * thread-safe. However, since the ConcurrentHashMap iterator is weakly consistent, it does not
+ * block new locations from being cached. If new locations are added while this is executing, they
+ * may be immediately invalidated by this code. Multiple calls to this method in different threads
+ * may cause some location caches to be invalidated multiple times. That is okay, because cache
+ * invalidation is idempotent.
+ */
+ public void clearTabletLocationCache() {
+ tabletLocationCache.forEach((dataLevel, map) -> {
+ // use iter.remove() instead of calling clear() on the map, to prevent clearing entries that
+ // may not have been invalidated
+ var iter = map.values().iterator();
+ while (iter.hasNext()) {
+ iter.next().invalidate();
+ iter.remove();
Review Comment:
Created #5434
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2014939805
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1098,43 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ return tabletLocationCache.get(DataLevel.of(tableId)).computeIfAbsent(tableId,
+ (TableId key) -> {
+ var lockChecker = getTServerLockChecker();
+ if (AccumuloTable.ROOT.tableId().equals(tableId)) {
+ return new RootClientTabletCache(lockChecker);
+ }
+ var mlo = new MetadataCachedTabletObtainer();
+ if (AccumuloTable.METADATA.tableId().equals(tableId)) {
+ return new ClientTabletCacheImpl(AccumuloTable.METADATA.tableId(),
+ getTabletLocationCache(AccumuloTable.ROOT.tableId()), mlo, lockChecker);
+ } else {
+ return new ClientTabletCacheImpl(tableId,
+ getTabletLocationCache(AccumuloTable.METADATA.tableId()), mlo, lockChecker);
+ }
+ });
+ }
+
+ /**
+ * Clear the currently cached tablet locations. The use of ConcurrentHashMap ensures this is
+ * thread-safe. However, since the ConcurrentHashMap iterator is weakly consistent, it does not
+ * block new locations from being cached. If new locations are added while this is executing, they
+ * may be immediately invalidated by this code. Multiple calls to this method in different threads
+ * may cause some location caches to be invalidated multiple times. That is okay, because cache
+ * invalidation is idempotent.
+ */
+ public void clearTabletLocationCache() {
+ tabletLocationCache.forEach((dataLevel, map) -> {
+ // use iter.remove() instead of calling clear() on the map, to prevent clearing entries that
+ // may not have been invalidated
+ var iter = map.values().iterator();
+ while (iter.hasNext()) {
+ iter.next().invalidate();
+ iter.remove();
Review Comment:
Are you saying that callers will hold onto ClientTabletCache objects retrieved from here, and keep using it after it is invalidated?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2015052826
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1098,43 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ return tabletLocationCache.get(DataLevel.of(tableId)).computeIfAbsent(tableId,
+ (TableId key) -> {
+ var lockChecker = getTServerLockChecker();
+ if (AccumuloTable.ROOT.tableId().equals(tableId)) {
+ return new RootClientTabletCache(lockChecker);
+ }
+ var mlo = new MetadataCachedTabletObtainer();
+ if (AccumuloTable.METADATA.tableId().equals(tableId)) {
+ return new ClientTabletCacheImpl(AccumuloTable.METADATA.tableId(),
+ getTabletLocationCache(AccumuloTable.ROOT.tableId()), mlo, lockChecker);
+ } else {
+ return new ClientTabletCacheImpl(tableId,
+ getTabletLocationCache(AccumuloTable.METADATA.tableId()), mlo, lockChecker);
+ }
+ });
+ }
+
+ /**
+ * Clear the currently cached tablet locations. The use of ConcurrentHashMap ensures this is
+ * thread-safe. However, since the ConcurrentHashMap iterator is weakly consistent, it does not
+ * block new locations from being cached. If new locations are added while this is executing, they
+ * may be immediately invalidated by this code. Multiple calls to this method in different threads
+ * may cause some location caches to be invalidated multiple times. That is okay, because cache
+ * invalidation is idempotent.
+ */
+ public void clearTabletLocationCache() {
+ tabletLocationCache.forEach((dataLevel, map) -> {
+ // use iter.remove() instead of calling clear() on the map, to prevent clearing entries that
+ // may not have been invalidated
+ var iter = map.values().iterator();
+ while (iter.hasNext()) {
+ iter.next().invalidate();
+ iter.remove();
Review Comment:
The BatchWriter, BatchScanner, and ConditoinalWriter get a reference to a tablet locator and then keep it. So then something like the following could happen
1. Create a BatchWriter which gets a ref to a cache C1 for table TAB1.
2. clearTabletLocationCache() is called
3. Create a BatchWriter which gets a ref to a cache C2 for table TAB1
So now there two batch writers that reference two different cache instances for the same table. Things probably should not hold on to references to the cache. The code seems to do that because it wraps the cache w/ decorator objects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #5421:
URL: https://github.com/apache/accumulo/pull/5421
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#issuecomment-2738513521
I think there is still more cleanup needed in the location cache code, even if this is a start.
I'm wondering if one big location cache (still separate obtainers based on tableId) is better than having hierarchical caches that are tightly coupled to their obtainers. Even if we don't do that, I think some of the naming and APIs could be cleaned up and I'm a bit concerned about the way the ZookeeperLockChecker is used by these, and has its own ZooSession/ZooCache. It can probably use the context's own ZooSession/ZooCache now, but I'm not quite sure, since these obtainers may still spin off background threads that outlive the context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#issuecomment-2738501166
This was created based on @keith-turner 's comment at https://github.com/apache/accumulo/pull/5282#discussion_r1929331849
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2017622266
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1098,43 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ return tabletLocationCache.get(DataLevel.of(tableId)).computeIfAbsent(tableId,
+ (TableId key) -> {
+ var lockChecker = getTServerLockChecker();
+ if (AccumuloTable.ROOT.tableId().equals(tableId)) {
+ return new RootClientTabletCache(lockChecker);
+ }
+ var mlo = new MetadataCachedTabletObtainer();
+ if (AccumuloTable.METADATA.tableId().equals(tableId)) {
+ return new ClientTabletCacheImpl(AccumuloTable.METADATA.tableId(),
+ getTabletLocationCache(AccumuloTable.ROOT.tableId()), mlo, lockChecker);
+ } else {
+ return new ClientTabletCacheImpl(tableId,
+ getTabletLocationCache(AccumuloTable.METADATA.tableId()), mlo, lockChecker);
+ }
+ });
+ }
+
+ /**
+ * Clear the currently cached tablet locations. The use of ConcurrentHashMap ensures this is
+ * thread-safe. However, since the ConcurrentHashMap iterator is weakly consistent, it does not
+ * block new locations from being cached. If new locations are added while this is executing, they
+ * may be immediately invalidated by this code. Multiple calls to this method in different threads
+ * may cause some location caches to be invalidated multiple times. That is okay, because cache
+ * invalidation is idempotent.
+ */
+ public void clearTabletLocationCache() {
+ tabletLocationCache.forEach((dataLevel, map) -> {
+ // use iter.remove() instead of calling clear() on the map, to prevent clearing entries that
+ // may not have been invalidated
+ var iter = map.values().iterator();
+ while (iter.hasNext()) {
+ iter.next().invalidate();
+ iter.remove();
Review Comment:
It's probably okay to have a reference to the old cache, but only if it notices that it was invalidated and replaces it before it tries to use it again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move static location cache methods to ClientContext [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5421:
URL: https://github.com/apache/accumulo/pull/5421#discussion_r2013286949
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1088,9 +1091,32 @@ public NamespaceMapping getNamespaces() {
return namespaces;
}
- public HashMap<TableId,ClientTabletCache> tabletCaches() {
+ public ClientTabletCache getTabletLocationCache(TableId tableId) {
ensureOpen();
- return tabletCaches;
+ synchronized (tabletLocationCache) {
Review Comment:
So, I think this synchronized is necessary to prevent new cache entries from being added. The sync mechanism on computeIfAbsent uses an internal lock object, and does not sync on the map itself. So, if we rely on synchronizing on the map itself during the cache invalidation method, without this synchronized block, then it's possible that new items can be added that will get cleared from the map, without ever having called `invalidate` on them. That's because methods like `values()` and `iterator()` on ConcurrentHashMap are weakly consistent, guaranteeing they will contain items when they are initially called... but not guaranteeing they won't see new items.
So, if we have to synchronize on the map to prevent additions, in order to safely ensure we have invalidated everything we remove, it kind of makes the whole purpose of using a ConcurrentHashMap go away.
We can still use ConcurrentHashMap, though... but we cannot call `map.clear()` in the `clearTabletLocationCache()` method. Instead, we have to create an iterator (weakly consistent) and call `invalidate()` and `iter.remove()` for each one. In that case, we don't have to do any synchronizing... we can just rely on the fact that the iterator is weakly consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org