Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/422#discussion_r204546734 --- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java --- @@ -170,55 +179,35 @@ public void processAdd(AddUpdateCommand cmd) throws IOException { final Instant routeTimestamp = parseRouteKey(routeValue); updateParsedCollectionAliases(); - String targetCollection; - do { // typically we don't loop; it's only when we need to create a collection - targetCollection = findTargetCollectionGivenTimestamp(routeTimestamp); - - if (targetCollection == null) { - throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, - "Doc " + cmd.getPrintableId() + " couldn't be routed with " + timeRoutedAlias.getRouteField() + "=" + routeTimestamp); - } - - // Note: the following rule is tempting but not necessary and is not compatible with - // only using this URP when the alias distrib phase is NONE; otherwise a doc may be routed to from a non-recent - // collection to the most recent only to then go there directly instead of realizing a new collection is needed. - // // If it's going to some other collection (not "this") then break to just send it there - // if (!thisCollection.equals(targetCollection)) { - // break; - // } - // Also tempting but not compatible: check that we're the leader, if not then break - - // If the doc goes to the most recent collection then do some checks below, otherwise break the loop. - final Instant mostRecentCollTimestamp = parsedCollectionsDesc.get(0).getKey(); - final String mostRecentCollName = parsedCollectionsDesc.get(0).getValue(); - if (!mostRecentCollName.equals(targetCollection)) { - break; - } - - // Check the doc isn't too far in the future - final Instant maxFutureTime = Instant.now().plusMillis(timeRoutedAlias.getMaxFutureMs()); - if (routeTimestamp.isAfter(maxFutureTime)) { - throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, - "The document's time routed key of " + routeValue + " is too far in the future given " + - TimeRoutedAlias.ROUTER_MAX_FUTURE + "=" + timeRoutedAlias.getMaxFutureMs()); - } - - // Create a new collection? - final Instant nextCollTimestamp = timeRoutedAlias.computeNextCollTimestamp(mostRecentCollTimestamp); - if (routeTimestamp.isBefore(nextCollTimestamp)) { - break; // thus we don't need another collection - } - - createCollectionAfter(mostRecentCollName); // *should* throw if fails for some reason but... - final boolean updated = updateParsedCollectionAliases(); - if (!updated) { // thus we didn't make progress... - // this is not expected, even in known failure cases, but we check just in case - throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, - "We need to create a new time routed collection but for unknown reasons were unable to do so."); + String targetCollection = findCandidateCollectionGivenTimestamp(routeTimestamp, cmd.getPrintableId()); + try { + String preemptiveCreateWindow = timeRoutedAlias.getPreemptiveCreateWindow(); + if (StringUtils.isBlank(preemptiveCreateWindow) || // default: no pre-create + requiresCreateCollection(routeTimestamp, null)) { // or no collection exists for doc + targetCollection = new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(targetCollection); // then do it synchronously. + } else if (requiresCreateCollection(routeTimestamp, preemptiveCreateWindow )) { // else async... + String finalTargetCollection = targetCollection; + try { + asyncMaintainers.computeIfAbsent(timeRoutedAlias.getAliasName(),(alias) -> + singleThreadSingleTaskExecutor()).submit(() -> + new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(finalTargetCollection)); + } catch (RejectedExecutionException e) { + // Note: There are some esoteric cases where the pre-create interval is a lot longer than --- End diff -- Ah. I think we simply don't care if it was rejected; pre-emption is best-effort. Your log statement is good. Maybe we don't want an Executor that can possibly reject us? I imagine every doc within this window during the collection creation might incur an exception throw? Look at createCollectionAfter() -- see how it uses a semaphore does nothing if another thread already has the lock. That's what we'd ideally have here too, right? Perhaps instead of a separate Maintainer class, we don't have that but instead have createCollectionAfter take an async boolean? If async is true, the "else" side (lock is already held) can simply return. if async is true if it grabs the lock, then it can go submit the logic in an executor that _won't_ throw a rejected exception. The release() needs to be performed when the work is done and not before. WDYT?
--- --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org