jihoonson commented on a change in pull request #8272:
URL: https://github.com/apache/druid/pull/8272#discussion_r433623247
##########
File path:
extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheScheduler.java
##########
@@ -381,17 +412,59 @@ public String toString()
ENTRY_CLOSED
}
- public final class VersionedCache implements CacheState, AutoCloseable
+ /**
+ * This builder class holds a {@link VersionedCache} object,
+ * For handling failures while loading cache, it exposes close() method
which will close {@link CacheHandler}
+ */
+ public final class VersionedCacheBuilder
Review comment:
Why is this class not a `Closeable`? You can use the `try-resource`
clause instead of [doing
this](https://github.com/apache/druid/pull/8272/files#diff-4d4be3477c4c0924f89ef01c6a7362adR103).
##########
File path:
extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheScheduler.java
##########
@@ -381,17 +412,59 @@ public String toString()
ENTRY_CLOSED
}
- public final class VersionedCache implements CacheState, AutoCloseable
+ /**
+ * This builder class holds a {@link VersionedCache} object,
+ * For handling failures while loading cache, it exposes close() method
which will close {@link CacheHandler}
+ */
+ public final class VersionedCacheBuilder
+ {
+
+ public VersionedCache getVersionedCache()
+ {
+ return versionedCache;
+ }
+
+ private final VersionedCache versionedCache;
+
+ public VersionedCacheBuilder(
+ @Nullable EntryImpl<? extends ExtractionNamespace> entryId,
+ String version,
+ @Nullable CacheHandler cacheHandler
+ )
+ {
+ updatesStarted.incrementAndGet();
+ this.versionedCache = new VersionedCache(String.valueOf(entryId),
version, cacheHandler);
+ }
+
+ public void close()
+ {
+ if (versionedCache != null) {
+ versionedCache.cacheHandler.close();
Review comment:
`cacheHandler` is nullable in `VersionedCache`.
##########
File path:
extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheScheduler.java
##########
@@ -197,6 +199,34 @@ public void run()
}
}
+ /**
+ *
+ *
+ * @param entryId an object uniquely corresponding to the {@link
CacheScheduler.Entry},
+ * to create Cache from
+ * @param version version, associated with the cache
+ * @param newCacheEntries List of Pairs consisting of new cache entries
to be added to current Cache.
+ * @return a new {@link VersionedCache} by adding pairs to existing cache,
This operation is
+ * non atomic and for Off-Heap caches there's possibility this will lead
to a crash, in case
+ * of use-after-free.
+ */
+ public @Nullable VersionedCache createFromExistingCache(
+ @Nullable EntryImpl<? extends ExtractionNamespace> entryId,
+ String version,
+ List<Pair<String, String>> newCacheEntries
+ )
+ {
+ if (!(cacheStateHolder.get() instanceof VersionedCache)) {
+ return null;
+ }
+ VersionedCache state = (VersionedCache) cacheStateHolder.get();
+ ConcurrentMap<String, String> cache = state.cacheHandler.getCache();
Review comment:
With this change, the cache scheduler will use the same instance of
`cache` for new updates with incremental loading. This changes the current
behavior of atomic replacement of versioned snapshots and thus I'm tagging this
PR with "Design Review". IMO, this is ok since 1) it's beneficial for heap
usage and 2) the lookup version is not locked during query processing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
