[GitHub] [druid] jihoonson commented on a change in pull request #8272: Support incremental load in Druid lookups

[GitBox <gi...@apache.org>]

jihoonson commented on a change in pull request #8272:
URL: https://github.com/apache/druid/pull/8272#discussion_r433620400



##########
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();

Review comment:
       This gets double-incremented here and at [Line 555](https://github.com/apache/druid/pull/8272/files#diff-f2359bc636b6f71d70297829eb79dc01R555).

##########
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:
       This seems more like a holder rather than a builder. Suggest renaming to `VersionedCacheHolder`.

##########
File path: docs/development/extensions-core/lookups-cached-global.md
##########
@@ -337,7 +337,7 @@ The `simpleJson` lookupParseSpec does not take any parameters. It is simply a li
 
 ### JDBC lookup
 
-The JDBC lookups will poll a database to populate its local cache. If the `tsColumn` is set it must be able to accept comparisons in the format `'2015-01-01 00:00:00'`. For example, the following must be valid SQL for the table `SELECT * FROM some_lookup_table WHERE timestamp_column >  '2015-01-01 00:00:00'`. If `tsColumn` is set, the caching service will attempt to only poll values that were written *after* the last sync. If `tsColumn` is not set, the entire table is pulled every time.
+The JDBC lookups will poll a database to populate its local cache. If the `tsColumn` is set it must be able to accept comparisons in the format `'2015-01-01 00:00:00'`. For example, the following must be valid sql for the table `SELECT * FROM some_lookup_table WHERE timestamp_column >  '2015-01-01 00:00:00'`. **If `tsColumn` is set, the caching service will attempt to only poll values that were written after the last sync, and any entries deleted from the the database won't be removed from the cache.** If `tsColumn` is not set, the entire table is pulled every time. Due to possibilities of use-after-free race conditions, **setting `tsColumn` is not recommended for `offHeap` lookups.**

Review comment:
       Please add more details about why `tsColumn` is not recommended so that users can be aware of what would happen if they use.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org