jnturton commented on code in PR #2929:
URL: https://github.com/apache/drill/pull/2929#discussion_r1794565832
##########
contrib/storage-splunk/src/main/java/org/apache/drill/exec/store/splunk/SplunkSchema.java:
##########
@@ -94,6 +109,10 @@ public CreateTableEntry createNewTable(String tableName,
List<String> partitionC
.message(plugin.getName() + " is not writable.")
.build(logger);
}
+ // Clear the index cache.
+ if (useCache) {
+ cache.invalidate(getNameForCache());
Review Comment:
It feels like it would be more natural (and efficient) for the cache to hold
one entry per Splunk index, rather than a single entry containing the list of
all indexes. Is there a reason it isn't built that way?
##########
contrib/storage-splunk/src/main/java/org/apache/drill/exec/store/splunk/SplunkSchema.java:
##########
@@ -38,19 +40,32 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.TimeUnit;
public class SplunkSchema extends AbstractSchema {
private final static Logger logger =
LoggerFactory.getLogger(SplunkSchema.class);
private static final String SPL_TABLE_NAME = "spl";
private final Map<String, DynamicDrillTable> activeTables = new HashMap<>();
private final SplunkStoragePlugin plugin;
private final String queryUserName;
+ private final Cache<String, Set<String>> cache;
+ private final boolean useCache;
public SplunkSchema(SplunkStoragePlugin plugin, String queryUserName) {
super(Collections.emptyList(), plugin.getName());
this.plugin = plugin;
this.queryUserName = queryUserName;
-
+ this.useCache = plugin.getConfig().getCacheExpiration() >= 0;
+
+ if (useCache) {
+ logger.debug("Using splunk schema cache for {}", plugin.getName());
Review Comment:
```suggestion
logger.info("Using splunk schema cache for {}", plugin.getName());
```
##########
contrib/storage-splunk/pom.xml:
##########
@@ -58,6 +58,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>com.github.ben-manes.caffeine</groupId>
+ <artifactId>caffeine</artifactId>
+ <version>2.9.3</version>
Review Comment:
Please lift the `caffeine.version` property from
metastore/iceberg-metastore/pom.xml to the root pom and either
- add caffeine with `caffeine.version` to dependencyManagement in the root
pom and remove version numbers here and in the Iceberg metastore, or
- make both this pom and Iceberg metastore pom specify
`<version>${caeffine.version}</version>`.
So that we standardise the version of Caffeine that gets pulled in.
--
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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
