a2l007 commented on a change in pull request #10421:
URL: https://github.com/apache/druid/pull/10421#discussion_r510222228
##########
File path:
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java
##########
@@ -168,7 +168,7 @@ public void start()
while (!Thread.interrupted() && lifecycleLock.awaitStarted(1,
TimeUnit.MILLISECONDS)) {
try {
handlePendingNotices();
- LockSupport.parkNanos(LookupReferencesManager.this,
TimeUnit.MINUTES.toNanos(1));
+ LockSupport.parkNanos(LookupReferencesManager.this,
TimeUnit.SECONDS.toNanos(1));
Review comment:
I feel that per second is more aggressive given that lookup updates may
not happen that frequently in most cases and per minute is more reasonable
since it is consistent with the default run period of the coordinator.
##########
File path:
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java
##########
@@ -587,6 +594,43 @@ private StringFullResponseHolder
fetchLookupsForTier(String tier) throws Interru
void handle(Map<String, LookupExtractorFactoryContainer> lookupMap) throws
Exception;
}
+ private static class StatusNotice implements Notice
Review comment:
Could you please add comments for this noticetype?
##########
File path:
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java
##########
@@ -587,6 +594,43 @@ private StringFullResponseHolder
fetchLookupsForTier(String tier) throws Interru
void handle(Map<String, LookupExtractorFactoryContainer> lookupMap) throws
Exception;
}
+ private static class StatusNotice implements Notice
+ {
+ private final LookupReferencesManager manager;
+ private final String lookupName;
+ private final LookupExtractorFactoryContainer
lookupExtractorFactoryContainer;
+
+ public StatusNotice(
+ LookupReferencesManager manager,
+ String lookupName,
+ LookupExtractorFactoryContainer lookupExtractorFactoryContainer
+ )
+ {
+ this.manager = manager;
+ this.lookupName = lookupName;
+ this.lookupExtractorFactoryContainer = lookupExtractorFactoryContainer;
+ }
+
+ @Override
+ public void handle(Map<String, LookupExtractorFactoryContainer> lookupMap)
+ {
+ if
(lookupExtractorFactoryContainer.getLookupExtractorFactory().isReady()) {
+ LookupExtractorFactoryContainer old = lookupMap.put(lookupName,
lookupExtractorFactoryContainer);
+
+ LOG.info("Loaded lookup [%s] with spec [%s].", lookupName,
lookupExtractorFactoryContainer);
+
+ if (old != null) {
+ if (!old.getLookupExtractorFactory().destroy()) {
+ throw new ISE("destroy method returned false for lookup
[%s]:[%s]", lookupName, old);
+ }
+ }
+ } else {
+ LOG.info("Loading lookup [%s] with spec [%s].", lookupName,
lookupExtractorFactoryContainer);
+ manager.addNotice(new StatusNotice(manager, lookupName,
lookupExtractorFactoryContainer));
Review comment:
Thanks.
> This PR only tries to improve the use case of updating an existing lookup
with higher version.
Could you please add tests for this? It should also help with the fixing the
code coverage failures in Travis.
----------------------------------------------------------------
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]
