Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r214122537
--- Diff:
solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java
---
@@ -165,31 +165,42 @@ private String getAliasName() {
@Override
public void processAdd(AddUpdateCommand cmd) throws IOException {
- SolrInputDocument solrInputDocument = cmd.getSolrInputDocument();
- final Object routeValue =
solrInputDocument.getFieldValue(timeRoutedAlias.getRouteField());
- final Instant docTimestampToRoute = parseRouteKey(routeValue);
- updateParsedCollectionAliases();
- String candidateCollection =
findCandidateCollectionGivenTimestamp(docTimestampToRoute,
cmd.getPrintableId());
- final Instant maxFutureTime =
Instant.now().plusMillis(timeRoutedAlias.getMaxFutureMs());
+ final Instant docTimestamp =
+
parseRouteKey(cmd.getSolrInputDocument().getFieldValue(timeRoutedAlias.getRouteField()));
+
// TODO: maybe in some cases the user would want to ignore/warn
instead?
- if (docTimestampToRoute.isAfter(maxFutureTime)) {
+ if
(docTimestamp.isAfter(Instant.now().plusMillis(timeRoutedAlias.getMaxFutureMs())))
{
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
- "The document's time routed key of " + docTimestampToRoute + "
is too far in the future given " +
+ "The document's time routed key of " + docTimestamp + " is too
far in the future given " +
TimeRoutedAlias.ROUTER_MAX_FUTURE + "=" +
timeRoutedAlias.getMaxFutureMs());
}
- String targetCollection =
createCollectionsIfRequired(docTimestampToRoute, candidateCollection,
cmd.getPrintableId());
+
+ // to avoid potential for race conditions, this next method should not
get called again unless
+ // we have created a collection synchronously
+ updateParsedCollectionAliases();
+
+ String targetCollection = createCollectionsIfRequired(docTimestamp,
cmd.getPrintableId(), cmd);
+
if (thisCollection.equals(targetCollection)) {
// pass on through; we've reached the right collection
super.processAdd(cmd);
} else {
// send to the right collection
- SolrCmdDistributor.Node targetLeaderNode =
routeDocToSlice(targetCollection, solrInputDocument);
+ SolrCmdDistributor.Node targetLeaderNode =
routeDocToSlice(targetCollection, cmd.getSolrInputDocument());
cmdDistrib.distribAdd(cmd,
Collections.singletonList(targetLeaderNode), new
ModifiableSolrParams(outParamsToLeader));
}
}
-
- private String createCollectionsIfRequired(Instant docTimestamp, String
targetCollection, String printableId) {
+ /**
+ * Create any required collections and return the name of the collection
to which the current document should be sent.
+ *
+ * @param docTimestamp the date for the document taken from the field
specified in the TRA config
--- End diff --
These parameter-level docs are fine but FYI don't feel that you have to do
this, especially for private methods, and especially for obvious parameters
(i.e. you don't *have* to document all; you could just do some at your
discretion, or none). This is subjective of course; I'm sharing my opinion and
not insisting on anything. My preference leans towards... "if you having
something helpful to say then say it, but avoid writing the obvious" (for
various reasons)
Oh look... you don't need to pass printableId since you could get it from
AddUpdateCommand if needed. printableId is a wart on these method signatures
IMO.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
