Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r214171837
--- Diff:
solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java
---
@@ -198,32 +209,32 @@ private String createCollectionsIfRequired(Instant
docTimestamp, String targetCo
// probably don't write to the same alias. As such, we have deferred
any solution to the "many clients causing
// collection creation simultaneously" problem until such time as
someone actually has that problem in a
// real world use case that isn't just an anti-pattern.
+ Map.Entry<Instant, String> candidateCollectionDesc =
findCandidateGivenTimestamp(docTimestamp, cmd.getPrintableId());
+ String candidateCollectionName = candidateCollectionDesc.getValue();
try {
- CreationType creationType = requiresCreateCollection(docTimestamp,
timeRoutedAlias.getPreemptiveCreateWindow());
- switch (creationType) {
+ switch (typeOfCreationRequired(docTimestamp,
candidateCollectionDesc.getKey())) {
case SYNCHRONOUS:
// This next line blocks until all collections required by the
current document have been created
- return maintain(targetCollection, docTimestamp, printableId,
false);
+ return createAllRequiredCollections(docTimestamp, printableId,
candidateCollectionDesc);
case ASYNC_PREEMPTIVE:
- // Note: creating an executor and throwing it away is slightly
expensive, but this is only likely to happen
- // once per hour/day/week (depending on time slice size for the
TRA). If the executor were retained, it
- // would need to be shut down in a close hook to avoid test
failures due to thread leaks which is slightly
- // more complicated from a code maintenance and readability
stand point. An executor must used instead of a
- // thread to ensure we pick up the proper MDC logging stuff from
ExecutorUtil. T
if (preemptiveCreationExecutor == null) {
- DefaultSolrThreadFactory threadFactory = new
DefaultSolrThreadFactory("TRA-preemptive-creation");
- preemptiveCreationExecutor =
newMDCAwareSingleThreadExecutor(threadFactory);
- preemptiveCreationExecutor.execute(() -> {
- maintain(targetCollection, docTimestamp, printableId, true);
- preemptiveCreationExecutor.shutdown();
- preemptiveCreationExecutor = null;
- });
+ // It's important not to add code between here and the prior
call to findCandidateGivenTimestamp()
+ // in processAdd() that invokes
updateParsedCollectionAliases(). Doing so would update parsedCollectionsDesc
+ // and create a race condition. We are relying on the fact
that get(0) is returning the head of the parsed
+ // collections that existed when candidateCollectionDesc was
created. If this class updates it's notion of
+ // parsedCollectionsDesc since candidateCollectionDesc was
chosen, we could create collection n+2
+ // instead of collection n+1.
+
+ // This line does not block and the document can be added
immediately
+ preemptiveAsync(() ->
createNextCollection(this.parsedCollectionsDesc.get(0).getValue()));
--- End diff --
argh I fixed this at one point (as evidenced by my comment) and somehow it
got back in there. I think it got reconsolidated was when I fixed the async
during sync bug (for which I added another section to the test).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
