Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212802141
--- Diff:
solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java
---
@@ -167,59 +167,17 @@ private String getAliasName() {
public void processAdd(AddUpdateCommand cmd) throws IOException {
SolrInputDocument solrInputDocument = cmd.getSolrInputDocument();
final Object routeValue =
solrInputDocument.getFieldValue(timeRoutedAlias.getRouteField());
- final Instant routeTimestamp = parseRouteKey(routeValue);
-
+ final Instant docTimestampToRoute = 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.");
- }
- // then retry the loop ...
- } while(true);
- assert targetCollection != null;
-
+ String candidateCollection =
findCandidateCollectionGivenTimestamp(docTimestampToRoute,
cmd.getPrintableId());
--- End diff --
You can move this line to immediately before its first use of the result.
Presently, the maxFutureTime check is inbetween which breaks up the natural
flow.
Hmm; even the "updateParsedCollectionAliases()" call can move down.
Finally, some newlines here & there would help separate separate steps.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
