Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r214121353
--- 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 --
bug: do not refer to this.parsedCollectionsDesc from the lambda runnable as
it is not thread-safe (and I don't think we should try to make it so. the
input should be gathered in the immediate line prior (thus in the main thread)
so that it's already resolved.
To make this bug harder (you did it twice), you could change
preemptiveAsync() to take the collection name instead of taking a lambda. It
could then be named createNextCollectionAsync
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
