You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by nsoft <gi...@git.apache.org> on 2018/08/08 02:36:28 UTC
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
GitHub user nsoft opened a pull request:
https://github.com/apache/lucene-solr/pull/433
SOLR-12357 Premptive creation of collections in Time Routed Aliases
new PR to clean up the strange merge conflicts
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nsoft/lucene-solr 12357-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucene-solr/pull/433.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #433
----
commit e4668036631adc7ecd094c6fd6b588d0f457ae1f
Author: Gus Heck <gu...@...>
Date: 2018-07-19T03:29:14Z
SOLR-12357 Premptively create collections in time routed aliases
(cherry picked from commit dcdc7c7)
commit c9f9872f79b84705daeb24ed6994c057d7d03de7
Author: Gus Heck <gu...@...>
Date: 2018-07-28T20:54:06Z
SOLR-12357 - Review Feedback, properly handle a preemptive
creation interval that happens to be longer than the time
slices for the TRA, further cleanup/organization.
(cherry picked from commit d07fcb4)
commit 613f8d50c1f1267c89152c140678e5243119bff1
Author: Gus Heck <gu...@...>
Date: 2018-08-08T01:08:24Z
SOLR-12357 - Simplified, removing static fields based on the realization
that the code path is rare and the case they guarded against is already
well known as an anti-pattern. Also moved some validation into the
TimeRoutedAlias class.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212680119
--- 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);
--- End diff --
elsewhere I notice you used "docTimestamp" which seems better to me than this long name.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212805945
--- 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 --
+1
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/lucene-solr/pull/433
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208716790
--- Diff: solr/core/src/java/org/apache/solr/cloud/api/collections/TimeRoutedAlias.java ---
@@ -141,6 +145,9 @@ public TimeRoutedAlias(String aliasName, Map<String, String> aliasMetadata) {
//optional:
maxFutureMs = params.getLong(ROUTER_MAX_FUTURE, TimeUnit.MINUTES.toMillis(10));
+ // the date math configured is an interval to be subtracted from the most recent collection's time stamp
+ preemptiveCreateMath = params.get(ROUTER_PREEMPTIVE_CREATE_MATH) != null ?
--- End diff --
Perhaps this should detect if it already starts with a plus or minus and if so removes it, thus making it more friendly to people who like to include the operator?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208449633
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -405,4 +449,58 @@ protected void doClose() {
collection, slice.getName());
}
+
+ /**
+ * Create as many collections as required. This method loops to allow for the possibility that the routeTimestamp
+ * requires more than one collection to be created. Since multiple threads may be invoking maintain on separate
+ * requests to the same alias, we must pass in the name of the collection that this thread believes to be the most
+ * recent collection. This assumption is checked when the command is executed in the overseer. When this method
+ * finds that all collections required have been created it returns the (possibly new) most recent collection.
+ * The return value is ignored by the calling code in the async preemptive case.
+ *
+ * @param targetCollection the initial notion of the latest collection available.
+ * @param docTimestamp the timestamp from the document that determines routing
+ * @param printableId an identifier for the add command used in error messages
+ * @return The latest collection, including collections created during maintenance
+ */
+ public String maintain(String targetCollection, Instant docTimestamp, String printableId) {
+ do { // typically we don't loop; it's only when we need to create a collection
+
+ // Note: This code no longer short circuits immediately when it sees that the expected latest
+ // collection is the current latest collection. With the advent of preemptive collection creation
+ // we always need to do the time based checks. Otherwise, we cannot handle the case where the
+ // preemptive window is larger than our TRA's time slices
+
+ // Check the doc isn't too far in the future
+ // TODO: Instant.now() here seems wrong...
+ final Instant maxFutureTime = Instant.now().plusMillis(timeRoutedAlias.getMaxFutureMs());
--- End diff --
I think we can move this above the loop or maybe even super up-front. It's documented in the ref guide and isn't tied to being associated with async/sync or near the edge. If someone doesn't want this safety check they can initialize it in a way as to have no practical effect (and sure, ought to be more easily disabled by using say null or -1).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r214172726
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -437,45 +461,46 @@ protected void doClose() {
/**
- * Create as many collections as required. This method loops to allow for the possibility that the routeTimestamp
+ * Create as many collections as required. This method loops to allow for the possibility that the docTimestamp
* requires more than one collection to be created. Since multiple threads may be invoking maintain on separate
* requests to the same alias, we must pass in the name of the collection that this thread believes to be the most
* recent collection. This assumption is checked when the command is executed in the overseer. When this method
* finds that all collections required have been created it returns the (possibly new) most recent collection.
* The return value is ignored by the calling code in the async preemptive case.
*
- * @param targetCollection the initial notion of the latest collection available.
* @param docTimestamp the timestamp from the document that determines routing
* @param printableId an identifier for the add command used in error messages
+ * @param targetCollectionDesc the descriptor for the presently selected collection which should also be
+ * the most recent collection in all cases where this method is invoked.
* @return The latest collection, including collections created during maintenance
*/
- public String maintain(String targetCollection, Instant docTimestamp, String printableId, boolean asyncSinglePassOnly) {
- do { // typically we don't loop; it's only when we need to create a collection
-
- // Note: This code no longer short circuits immediately when it sees that the expected latest
- // collection is the current latest collection. With the advent of preemptive collection creation
- // we always need to do the time based checks. Otherwise, we cannot handle the case where the
- // preemptive window is larger than our TRA's time slices
-
- // Check the doc isn't too far in the future
-
- if (NONE == requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow()))
- return targetCollection; // thus we don't need another collection
+ private String createAllRequiredCollections( Instant docTimestamp, String printableId,
+ Map.Entry<Instant, String> targetCollectionDesc) {
+ do {
+ switch(typeOfCreationRequired(docTimestamp, targetCollectionDesc.getKey())) {
+ case NONE:
+ return targetCollectionDesc.getValue(); // we don't need another collection
+ case ASYNC_PREEMPTIVE:
+ // can happen when preemptive interval is longer than one time slice
+ preemptiveAsync(() -> createNextCollection(this.parsedCollectionsDesc.get(0).getValue()));
+ return targetCollectionDesc.getValue();
+ case SYNCHRONOUS:
+ createNextCollection(targetCollectionDesc.getValue()); // *should* throw if fails for some reason but...
+ if (!updateParsedCollectionAliases()) { // 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 ... have to do find again in case other requests also added collections
+ // that were made visible when we called updateParsedCollectionAliases()
+ targetCollectionDesc = findCandidateGivenTimestamp(docTimestamp, printableId);
+ break;
+ default:
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown creation type while adding " +
--- End diff --
no not required, but I had a wrong default return targetCollectionDesc.getValue() previously and replaced it with this. The assert would be fine too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212677139
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -405,4 +435,54 @@ protected void doClose() {
collection, slice.getName(), DistributedUpdateProcessor.MAX_RETRIES_ON_FORWARD_DEAULT);
}
+
+ /**
+ * Create as many collections as required. This method loops to allow for the possibility that the routeTimestamp
+ * requires more than one collection to be created. Since multiple threads may be invoking maintain on separate
+ * requests to the same alias, we must pass in the name of the collection that this thread believes to be the most
+ * recent collection. This assumption is checked when the command is executed in the overseer. When this method
+ * finds that all collections required have been created it returns the (possibly new) most recent collection.
+ * The return value is ignored by the calling code in the async preemptive case.
+ *
+ * @param targetCollection the initial notion of the latest collection available.
+ * @param docTimestamp the timestamp from the document that determines routing
+ * @param printableId an identifier for the add command used in error messages
+ * @return The latest collection, including collections created during maintenance
+ */
+ public String maintain(String targetCollection, Instant docTimestamp, String printableId, boolean asyncSinglePassOnly) {
+ do { // typically we don't loop; it's only when we need to create a collection
+
+ // Note: This code no longer short circuits immediately when it sees that the expected latest
--- End diff --
I don't think source code should refer to what the code used to do. You could communicate this in GitHub/JIRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208446107
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -167,66 +173,121 @@ 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);
+ String candidateCollection = findCandidateCollectionGivenTimestamp(docTimestampToRoute, cmd.getPrintableId());
+ String targetCollection = createCollectionsIfRequired(docTimestampToRoute, candidateCollection, cmd.getPrintableId());
+ 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);
+ cmdDistrib.distribAdd(cmd, Collections.singletonList(targetLeaderNode), new ModifiableSolrParams(outParamsToLeader));
+ }
+ }
- if (targetCollection == null) {
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
- "Doc " + cmd.getPrintableId() + " couldn't be routed with " + timeRoutedAlias.getRouteField() + "=" + routeTimestamp);
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId);
+ case ASYNC_PREEMPTIVE:
+ // Note: creating an executor is slightly expensive, but only likely to happen once per hour/day/week
+ // (depending on time slice size for the TRA). Executor is used to ensure we pick up the MDC logging stuff
+ // from ExecutorUtil. Even though it is possible that multiple requests hit this code in the 1-2 sec that
+ // it takes to create a collection, it's an established anti-pattern to feed data with a very large number
+ // of client connections. This in mind, we only guard against spamming the overseer within a batch of
+ // updates, intentionally tolerating a low level of redundant requests in favor of simpler code. Most
+ // super-sized installations with many update clients will likely be multi-tenant and multiple tenants
+ // probably don't write to the same alias. As such, we have deferred any solution 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.
+ synchronized (execLock) {
+ if (preemptiveCreationExecutor == null) {
+ preemptiveCreationExecutor = preemptiveCreationExecutor();
+ }
+ preemptiveCreationExecutor.execute(() -> {
+ maintain(targetCollection, docTimestamp, printableId);
+ preemptiveCreationExecutor = null;
+ });
+ preemptiveCreationExecutor.shutdown(); // shutdown immediately to ensure no new requests accepted
--- End diff --
Hmmm; I think we can shut down in finish() if it's non-null. Wouldn't we want to keep this active for hypothetical more pre-emptive creation during the lifespan of this URP instance (however extremely unlikely that is I totally grant you). I think it'd be easier to reason about.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r213405495
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -322,6 +325,104 @@ public void testSliceRouting() throws Exception {
}
}
+ @Test
+ public void testPreemptiveCreation() throws Exception {
+ String configName = TimeRoutedAliasUpdateProcessorTest.configName + getTestName();
+ createConfigSet(configName);
+
+ final int numShards = 1 ;
+ final int numReplicas = 1 ;
+ CollectionAdminRequest.createTimeRoutedAlias(alias, "2017-10-23T00:00:00Z", "+1DAY", timeField,
+ CollectionAdminRequest.createCollection("_unused_", configName, numShards, numReplicas)
+ .setMaxShardsPerNode(numReplicas)).setPreemptiveCreateWindow("3HOUR")
+ .process(solrClient);
+
+ // cause some collections to be created
+ assertUpdateResponse(solrClient.add(alias,
+ sdoc("id","1","timestamp_dt", "2017-10-25T00:00:00Z")
--- End diff --
comments (now below) address the purpose of each collection
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212679352
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +188,95 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ // Even though it is possible that multiple requests hit this code in the 1-2 sec that
+ // it takes to create a collection, it's an established anti-pattern to feed data with a very large number
+ // of client connections. This in mind, we only guard against spamming the overseer within a batch of
+ // updates. We are intentionally tolerating a low level of redundant requests in favor of simpler code. Most
+ // super-sized installations with many update clients will likely be multi-tenant and multiple tenants
+ // 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.
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId, false);
+ 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;
+ });
+ }
+ return targetCollection;
+ case NONE:
+ return targetCollection; // just for clarity...
+ default:
+ return targetCollection; // could use fall through, but fall through is fiddly for later editors.
+ }
+ // do nothing if creationType == NONE
+ } catch (SolrException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Determine if the a new collection will be required based on the document timestamp. Passing null for
+ * preemptiveCreateInterval tells you if the document is beyond all existing collections with a response of
+ * {@link CreationType#NONE} or {@link CreationType#SYNCHRONOUS}, and passing a valid date math for
+ * preemptiveCreateMath additionally distinguishes the case where the document is close enough to the end of
+ * the TRA to trigger preemptive creation but not beyond all existing collections with a value of
+ * {@link CreationType#ASYNC_PREEMPTIVE}.
+ *
+ * @param routeTimestamp The timestamp from the document
+ * @param preemptiveCreateMath The date math indicating the {@link TimeRoutedAlias#preemptiveCreateMath}
+ * @return a {@code CreationType} indicating if and how to create a collection
+ */
+ private CreationType requiresCreateCollection(Instant routeTimestamp, String preemptiveCreateMath) {
--- End diff --
don't need to pass preemptiveCreateMath... consider it's easily available from the timeRoutedAlias field which is already used in the code below. Since elsewhere you've renamed "routeTimestamp" to be "docTimestamp", perhaps rename that as well?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r213397248
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +188,95 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ // Even though it is possible that multiple requests hit this code in the 1-2 sec that
+ // it takes to create a collection, it's an established anti-pattern to feed data with a very large number
+ // of client connections. This in mind, we only guard against spamming the overseer within a batch of
+ // updates. We are intentionally tolerating a low level of redundant requests in favor of simpler code. Most
+ // super-sized installations with many update clients will likely be multi-tenant and multiple tenants
+ // 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.
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId, false);
+ 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;
+ });
+ }
+ return targetCollection;
+ case NONE:
+ return targetCollection; // just for clarity...
+ default:
+ return targetCollection; // could use fall through, but fall through is fiddly for later editors.
+ }
+ // do nothing if creationType == NONE
+ } catch (SolrException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Determine if the a new collection will be required based on the document timestamp. Passing null for
+ * preemptiveCreateInterval tells you if the document is beyond all existing collections with a response of
+ * {@link CreationType#NONE} or {@link CreationType#SYNCHRONOUS}, and passing a valid date math for
+ * preemptiveCreateMath additionally distinguishes the case where the document is close enough to the end of
+ * the TRA to trigger preemptive creation but not beyond all existing collections with a value of
+ * {@link CreationType#ASYNC_PREEMPTIVE}.
+ *
+ * @param routeTimestamp The timestamp from the document
+ * @param preemptiveCreateMath The date math indicating the {@link TimeRoutedAlias#preemptiveCreateMath}
+ * @return a {@code CreationType} indicating if and how to create a collection
+ */
+ private CreationType requiresCreateCollection(Instant routeTimestamp, String preemptiveCreateMath) {
--- End diff --
+1
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212806120
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +188,95 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ // Even though it is possible that multiple requests hit this code in the 1-2 sec that
+ // it takes to create a collection, it's an established anti-pattern to feed data with a very large number
+ // of client connections. This in mind, we only guard against spamming the overseer within a batch of
+ // updates. We are intentionally tolerating a low level of redundant requests in favor of simpler code. Most
+ // super-sized installations with many update clients will likely be multi-tenant and multiple tenants
+ // 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.
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId, false);
+ 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(() -> {
--- End diff --
I've not wanted to create 2 places in the code where we do the same thing, but I think I figured out how to factor it so it's both clearer and non-duplicative...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208718224
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -322,6 +325,104 @@ public void testSliceRouting() throws Exception {
}
}
+ @Test
+ public void testPreemptiveCreation() throws Exception {
+ String configName = TimeRoutedAliasUpdateProcessorTest.configName + getTestName();
+ createConfigSet(configName);
+
+ final int numShards = 1 ;
+ final int numReplicas = 1 ;
+ CollectionAdminRequest.createTimeRoutedAlias(alias, "2017-10-23T00:00:00Z", "+1DAY", timeField,
+ CollectionAdminRequest.createCollection("_unused_", configName, numShards, numReplicas)
+ .setMaxShardsPerNode(numReplicas)).setPreemptiveCreateWindow("3HOUR")
+ .process(solrClient);
+
+ // cause some collections to be created
+ assertUpdateResponse(solrClient.add(alias,
+ sdoc("id","1","timestamp_dt", "2017-10-25T00:00:00Z")
+ ));
+ assertUpdateResponse(solrClient.commit(alias));
+
+ // wait for all the collections to exist...
+ waitCol("2017-10-23", numShards);
+ waitCol("2017-10-24", numShards);
+ waitCol("2017-10-25", numShards);
+
+ // normal update, nothing special, no collection creation required.
+ List<String> cols = new CollectionAdminRequest.ListAliases().process(solrClient).getAliasesAsLists().get(alias);
+ assertEquals(3,cols.size());
+
+ assertNumDocs("2017-10-23", 0);
+ assertNumDocs("2017-10-24", 0);
+ assertNumDocs("2017-10-25", 1);
+
+ // cause some collections to be created
+
+ ModifiableSolrParams params = params();
+ assertUpdateResponse(add(alias, Arrays.asList(
+ sdoc("id", "2", "timestamp_dt", "2017-10-24T00:00:00Z"),
+ sdoc("id", "3", "timestamp_dt", "2017-10-25T00:00:00Z"),
+ sdoc("id", "4", "timestamp_dt", "2017-10-23T00:00:00Z"),
+ sdoc("id", "5", "timestamp_dt", "2017-10-25T23:00:00Z")), // should cause preemptive creation
+ params));
+ assertUpdateResponse(solrClient.commit(alias));
+
+ cols = new CollectionAdminRequest.ListAliases().process(solrClient).getAliasesAsLists().get(alias);
+ assertEquals(3,cols.size());
+ assertNumDocs("2017-10-23", 1);
+ assertNumDocs("2017-10-24", 1);
+ assertNumDocs("2017-10-25", 3);
+
+ assertUpdateResponse(add(alias, Collections.singletonList(
+ sdoc("id", "6", "timestamp_dt", "2017-10-25T23:01:00Z")), // might cause duplicate preemptive creation
+ params));
+ assertUpdateResponse(solrClient.commit(alias));
+
+ waitCol("2017-10-26", numShards);
+ cols = new CollectionAdminRequest.ListAliases().process(solrClient).getAliasesAsLists().get(alias);
+ assertEquals(4,cols.size());
+ assertNumDocs("2017-10-23", 1);
+ assertNumDocs("2017-10-24", 1);
+ assertNumDocs("2017-10-25", 4);
+ assertNumDocs("2017-10-26", 0);
+
+ // now test with pre-create window longer than time slice, and forcing multiple creations.
+ CollectionAdminRequest.setAliasProperty(alias)
+ .addProperty(TimeRoutedAlias.ROUTER_PREEMPTIVE_CREATE_MATH, "3DAY").process(solrClient);
+
+ Thread.sleep(1000); // a moment to be sure the alias change has taken effect
--- End diff --
curious; has this been shown to be necessary? I can imagine it might be... since a new TRA URP isn't doing to do a ZK sync to view the alias state.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r213398526
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +188,95 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ // Even though it is possible that multiple requests hit this code in the 1-2 sec that
+ // it takes to create a collection, it's an established anti-pattern to feed data with a very large number
+ // of client connections. This in mind, we only guard against spamming the overseer within a batch of
+ // updates. We are intentionally tolerating a low level of redundant requests in favor of simpler code. Most
+ // super-sized installations with many update clients will likely be multi-tenant and multiple tenants
+ // 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.
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId, false);
+ 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;
+ });
+ }
+ return targetCollection;
+ case NONE:
+ return targetCollection; // just for clarity...
+ default:
+ return targetCollection; // could use fall through, but fall through is fiddly for later editors.
+ }
+ // do nothing if creationType == NONE
+ } catch (SolrException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Determine if the a new collection will be required based on the document timestamp. Passing null for
+ * preemptiveCreateInterval tells you if the document is beyond all existing collections with a response of
+ * {@link CreationType#NONE} or {@link CreationType#SYNCHRONOUS}, and passing a valid date math for
+ * preemptiveCreateMath additionally distinguishes the case where the document is close enough to the end of
+ * the TRA to trigger preemptive creation but not beyond all existing collections with a value of
+ * {@link CreationType#ASYNC_PREEMPTIVE}.
+ *
+ * @param routeTimestamp The timestamp from the document
+ * @param preemptiveCreateMath The date math indicating the {@link TimeRoutedAlias#preemptiveCreateMath}
+ * @return a {@code CreationType} indicating if and how to create a collection
+ */
+ private CreationType requiresCreateCollection(Instant routeTimestamp, String preemptiveCreateMath) {
+ final Instant mostRecentCollTimestamp = parsedCollectionsDesc.get(0).getKey();
+ final Instant nextCollTimestamp = timeRoutedAlias.computeNextCollTimestamp(mostRecentCollTimestamp);
+ if (!routeTimestamp.isBefore(nextCollTimestamp)) {
+ // current document is destined for a collection that doesn't exist, must create the destination
+ // to proceed with this add command
+ return SYNCHRONOUS;
+ }
+
+ if (isBlank(preemptiveCreateMath)) {
--- End diff --
+1
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208445252
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -167,66 +173,121 @@ 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);
+ String candidateCollection = findCandidateCollectionGivenTimestamp(docTimestampToRoute, cmd.getPrintableId());
+ String targetCollection = createCollectionsIfRequired(docTimestampToRoute, candidateCollection, cmd.getPrintableId());
+ 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);
+ cmdDistrib.distribAdd(cmd, Collections.singletonList(targetLeaderNode), new ModifiableSolrParams(outParamsToLeader));
+ }
+ }
- if (targetCollection == null) {
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
- "Doc " + cmd.getPrintableId() + " couldn't be routed with " + timeRoutedAlias.getRouteField() + "=" + routeTimestamp);
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId);
+ case ASYNC_PREEMPTIVE:
+ // Note: creating an executor is slightly expensive, but only likely to happen once per hour/day/week
--- End diff --
ok but you can place this comment maybe somewhere higher as it applies to both sync & pre-emptive.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r214118388
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -437,45 +461,46 @@ protected void doClose() {
/**
- * Create as many collections as required. This method loops to allow for the possibility that the routeTimestamp
+ * Create as many collections as required. This method loops to allow for the possibility that the docTimestamp
* requires more than one collection to be created. Since multiple threads may be invoking maintain on separate
* requests to the same alias, we must pass in the name of the collection that this thread believes to be the most
* recent collection. This assumption is checked when the command is executed in the overseer. When this method
* finds that all collections required have been created it returns the (possibly new) most recent collection.
* The return value is ignored by the calling code in the async preemptive case.
*
- * @param targetCollection the initial notion of the latest collection available.
* @param docTimestamp the timestamp from the document that determines routing
* @param printableId an identifier for the add command used in error messages
+ * @param targetCollectionDesc the descriptor for the presently selected collection which should also be
+ * the most recent collection in all cases where this method is invoked.
* @return The latest collection, including collections created during maintenance
*/
- public String maintain(String targetCollection, Instant docTimestamp, String printableId, boolean asyncSinglePassOnly) {
- do { // typically we don't loop; it's only when we need to create a collection
-
- // Note: This code no longer short circuits immediately when it sees that the expected latest
- // collection is the current latest collection. With the advent of preemptive collection creation
- // we always need to do the time based checks. Otherwise, we cannot handle the case where the
- // preemptive window is larger than our TRA's time slices
-
- // Check the doc isn't too far in the future
-
- if (NONE == requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow()))
- return targetCollection; // thus we don't need another collection
+ private String createAllRequiredCollections( Instant docTimestamp, String printableId,
+ Map.Entry<Instant, String> targetCollectionDesc) {
+ do {
+ switch(typeOfCreationRequired(docTimestamp, targetCollectionDesc.getKey())) {
+ case NONE:
+ return targetCollectionDesc.getValue(); // we don't need another collection
+ case ASYNC_PREEMPTIVE:
+ // can happen when preemptive interval is longer than one time slice
+ preemptiveAsync(() -> createNextCollection(this.parsedCollectionsDesc.get(0).getValue()));
+ return targetCollectionDesc.getValue();
+ case SYNCHRONOUS:
+ createNextCollection(targetCollectionDesc.getValue()); // *should* throw if fails for some reason but...
+ if (!updateParsedCollectionAliases()) { // 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 ... have to do find again in case other requests also added collections
+ // that were made visible when we called updateParsedCollectionAliases()
+ targetCollectionDesc = findCandidateGivenTimestamp(docTimestamp, printableId);
+ break;
+ default:
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown creation type while adding " +
--- End diff --
I wouldn't waste your virtual breath (lines of code with thoughtful explanation) on a default case of an enum switch. Another approach I like is to `assert ENUMNAME.values().length == 3;` before the switch which will be caught at test time. I forget but if java still insists we have a default then throw IllegalStateException or something like that in a one-liner without explaination. (keep it short & sweet)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208720134
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -322,6 +325,104 @@ public void testSliceRouting() throws Exception {
}
}
+ @Test
+ public void testPreemptiveCreation() throws Exception {
+ String configName = TimeRoutedAliasUpdateProcessorTest.configName + getTestName();
+ createConfigSet(configName);
+
+ final int numShards = 1 ;
+ final int numReplicas = 1 ;
+ CollectionAdminRequest.createTimeRoutedAlias(alias, "2017-10-23T00:00:00Z", "+1DAY", timeField,
+ CollectionAdminRequest.createCollection("_unused_", configName, numShards, numReplicas)
+ .setMaxShardsPerNode(numReplicas)).setPreemptiveCreateWindow("3HOUR")
+ .process(solrClient);
+
+ // cause some collections to be created
+ assertUpdateResponse(solrClient.add(alias,
+ sdoc("id","1","timestamp_dt", "2017-10-25T00:00:00Z")
+ ));
+ assertUpdateResponse(solrClient.commit(alias));
+
+ // wait for all the collections to exist...
+ waitCol("2017-10-23", numShards);
+ waitCol("2017-10-24", numShards);
+ waitCol("2017-10-25", numShards);
+
+ // normal update, nothing special, no collection creation required.
+ List<String> cols = new CollectionAdminRequest.ListAliases().process(solrClient).getAliasesAsLists().get(alias);
+ assertEquals(3,cols.size());
+
+ assertNumDocs("2017-10-23", 0);
+ assertNumDocs("2017-10-24", 0);
+ assertNumDocs("2017-10-25", 1);
+
+ // cause some collections to be created
+
+ ModifiableSolrParams params = params();
+ assertUpdateResponse(add(alias, Arrays.asList(
+ sdoc("id", "2", "timestamp_dt", "2017-10-24T00:00:00Z"),
+ sdoc("id", "3", "timestamp_dt", "2017-10-25T00:00:00Z"),
+ sdoc("id", "4", "timestamp_dt", "2017-10-23T00:00:00Z"),
+ sdoc("id", "5", "timestamp_dt", "2017-10-25T23:00:00Z")), // should cause preemptive creation
--- End diff --
lets also add another document after this, one that would also cause pre-emptive creation if it weren't for there already being a request in-progress
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212809165
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -392,29 +393,73 @@ public void testPreemptiveCreation() throws Exception {
CollectionAdminRequest.setAliasProperty(alias)
.addProperty(TimeRoutedAlias.ROUTER_PREEMPTIVE_CREATE_MATH, "3DAY").process(solrClient);
- Thread.sleep(1000); // a moment to be sure the alias change has taken effect
-
assertUpdateResponse(add(alias, Collections.singletonList(
sdoc("id", "7", "timestamp_dt", "2017-10-25T23:01:00Z")), // should cause preemptive creation now
params));
assertUpdateResponse(solrClient.commit(alias));
waitCol("2017-10-27", numShards);
- waitCol("2017-10-28", numShards);
cols = new CollectionAdminRequest.ListAliases().process(solrClient).getAliasesAsLists().get(alias);
- assertEquals(6,cols.size());
+ assertEquals(5,cols.size()); // only one created in async case
assertNumDocs("2017-10-23", 1);
assertNumDocs("2017-10-24", 1);
assertNumDocs("2017-10-25", 5);
assertNumDocs("2017-10-26", 0);
assertNumDocs("2017-10-27", 0);
+
+ assertUpdateResponse(add(alias, Collections.singletonList(
+ sdoc("id", "8", "timestamp_dt", "2017-10-25T23:01:00Z")), // should cause preemptive creation now
+ params));
+ assertUpdateResponse(solrClient.commit(alias));
+ waitCol("2017-10-27", numShards);
+ waitCol("2017-10-28", numShards);
+
+ cols = new CollectionAdminRequest.ListAliases().process(solrClient).getAliasesAsLists().get(alias);
+ assertEquals(6,cols.size()); // Subsequent documents continue to create up to limit
+ assertNumDocs("2017-10-23", 1);
+ assertNumDocs("2017-10-24", 1);
+ assertNumDocs("2017-10-25", 6);
+ assertNumDocs("2017-10-26", 0);
+ assertNumDocs("2017-10-27", 0);
assertNumDocs("2017-10-28", 0);
QueryResponse resp;
resp = solrClient.query(alias, params(
"q", "*:*",
"rows", "10"));
- assertEquals(7, resp.getResults().getNumFound());
+ assertEquals(8, resp.getResults().getNumFound());
+
+ assertUpdateResponse(add(alias, Arrays.asList(
--- End diff --
Shouldn't we use `addDocsAndCommit` here and the other spots where multiple docs are added at a time? I know you're passing params but it's empty.
Otherwise, tests look good!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r214170180
--- 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 --
nice catch, yeah I added the cmd late in the game. very happy to get rid of printable id.
Generally if I doc a method I doc it fully (goal at least) if not I don't do any of it. I generally add to private methods only where it seems to be adding something. In this case the method name doesn't actually indicate that we're also selecting the target collection as well. The method name is already long so I used docs.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212805841
--- 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);
--- End diff --
+1
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r214173448
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -437,45 +461,46 @@ protected void doClose() {
/**
- * Create as many collections as required. This method loops to allow for the possibility that the routeTimestamp
+ * Create as many collections as required. This method loops to allow for the possibility that the docTimestamp
* requires more than one collection to be created. Since multiple threads may be invoking maintain on separate
* requests to the same alias, we must pass in the name of the collection that this thread believes to be the most
* recent collection. This assumption is checked when the command is executed in the overseer. When this method
* finds that all collections required have been created it returns the (possibly new) most recent collection.
* The return value is ignored by the calling code in the async preemptive case.
*
- * @param targetCollection the initial notion of the latest collection available.
* @param docTimestamp the timestamp from the document that determines routing
* @param printableId an identifier for the add command used in error messages
+ * @param targetCollectionDesc the descriptor for the presently selected collection which should also be
+ * the most recent collection in all cases where this method is invoked.
* @return The latest collection, including collections created during maintenance
*/
- public String maintain(String targetCollection, Instant docTimestamp, String printableId, boolean asyncSinglePassOnly) {
- do { // typically we don't loop; it's only when we need to create a collection
-
- // Note: This code no longer short circuits immediately when it sees that the expected latest
- // collection is the current latest collection. With the advent of preemptive collection creation
- // we always need to do the time based checks. Otherwise, we cannot handle the case where the
- // preemptive window is larger than our TRA's time slices
-
- // Check the doc isn't too far in the future
-
- if (NONE == requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow()))
- return targetCollection; // thus we don't need another collection
+ private String createAllRequiredCollections( Instant docTimestamp, String printableId,
+ Map.Entry<Instant, String> targetCollectionDesc) {
+ do {
+ switch(typeOfCreationRequired(docTimestamp, targetCollectionDesc.getKey())) {
+ case NONE:
+ return targetCollectionDesc.getValue(); // we don't need another collection
+ case ASYNC_PREEMPTIVE:
+ // can happen when preemptive interval is longer than one time slice
+ preemptiveAsync(() -> createNextCollection(this.parsedCollectionsDesc.get(0).getValue()));
--- End diff --
Agree see above comment
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r213403086
--- Diff: solr/core/src/java/org/apache/solr/cloud/api/collections/TimeRoutedAlias.java ---
@@ -141,6 +145,9 @@ public TimeRoutedAlias(String aliasName, Map<String, String> aliasMetadata) {
//optional:
maxFutureMs = params.getLong(ROUTER_MAX_FUTURE, TimeUnit.MINUTES.toMillis(10));
+ // the date math configured is an interval to be subtracted from the most recent collection's time stamp
+ preemptiveCreateMath = params.get(ROUTER_PREEMPTIVE_CREATE_MATH) != null ?
--- End diff --
ok
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212802686
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +188,95 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ // Even though it is possible that multiple requests hit this code in the 1-2 sec that
+ // it takes to create a collection, it's an established anti-pattern to feed data with a very large number
+ // of client connections. This in mind, we only guard against spamming the overseer within a batch of
+ // updates. We are intentionally tolerating a low level of redundant requests in favor of simpler code. Most
+ // super-sized installations with many update clients will likely be multi-tenant and multiple tenants
+ // 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.
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId, false);
+ 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(() -> {
--- End diff --
the code executed in the new thread should not call maintain() since I see you had to make maintain more complicated to tell if it's being called from "async". It can call:
```
final String mostRecentCollName = parsedCollectionsDesc.get(0).getValue();
createCollectionAfter(mostRecentCollName);
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r214199163
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -437,45 +461,46 @@ protected void doClose() {
/**
- * Create as many collections as required. This method loops to allow for the possibility that the routeTimestamp
+ * Create as many collections as required. This method loops to allow for the possibility that the docTimestamp
* requires more than one collection to be created. Since multiple threads may be invoking maintain on separate
* requests to the same alias, we must pass in the name of the collection that this thread believes to be the most
* recent collection. This assumption is checked when the command is executed in the overseer. When this method
* finds that all collections required have been created it returns the (possibly new) most recent collection.
* The return value is ignored by the calling code in the async preemptive case.
*
- * @param targetCollection the initial notion of the latest collection available.
* @param docTimestamp the timestamp from the document that determines routing
* @param printableId an identifier for the add command used in error messages
+ * @param targetCollectionDesc the descriptor for the presently selected collection which should also be
+ * the most recent collection in all cases where this method is invoked.
* @return The latest collection, including collections created during maintenance
*/
- public String maintain(String targetCollection, Instant docTimestamp, String printableId, boolean asyncSinglePassOnly) {
- do { // typically we don't loop; it's only when we need to create a collection
-
- // Note: This code no longer short circuits immediately when it sees that the expected latest
- // collection is the current latest collection. With the advent of preemptive collection creation
- // we always need to do the time based checks. Otherwise, we cannot handle the case where the
- // preemptive window is larger than our TRA's time slices
-
- // Check the doc isn't too far in the future
-
- if (NONE == requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow()))
- return targetCollection; // thus we don't need another collection
+ private String createAllRequiredCollections( Instant docTimestamp, String printableId,
+ Map.Entry<Instant, String> targetCollectionDesc) {
+ do {
+ switch(typeOfCreationRequired(docTimestamp, targetCollectionDesc.getKey())) {
+ case NONE:
+ return targetCollectionDesc.getValue(); // we don't need another collection
+ case ASYNC_PREEMPTIVE:
+ // can happen when preemptive interval is longer than one time slice
+ preemptiveAsync(() -> createNextCollection(this.parsedCollectionsDesc.get(0).getValue()));
+ return targetCollectionDesc.getValue();
+ case SYNCHRONOUS:
+ createNextCollection(targetCollectionDesc.getValue()); // *should* throw if fails for some reason but...
+ if (!updateParsedCollectionAliases()) { // 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 ... have to do find again in case other requests also added collections
+ // that were made visible when we called updateParsedCollectionAliases()
+ targetCollectionDesc = findCandidateGivenTimestamp(docTimestamp, printableId);
+ break;
+ default:
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown creation type while adding " +
--- End diff --
After playing with it I decided to just factor it into a helper method (reused in both places) since it's only real draw back is cluttering up the code visually. (I've already done the typing, and the assert just looks a bit odd to my eye)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208717805
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -322,6 +325,104 @@ public void testSliceRouting() throws Exception {
}
}
+ @Test
+ public void testPreemptiveCreation() throws Exception {
+ String configName = TimeRoutedAliasUpdateProcessorTest.configName + getTestName();
+ createConfigSet(configName);
+
+ final int numShards = 1 ;
+ final int numReplicas = 1 ;
+ CollectionAdminRequest.createTimeRoutedAlias(alias, "2017-10-23T00:00:00Z", "+1DAY", timeField,
+ CollectionAdminRequest.createCollection("_unused_", configName, numShards, numReplicas)
+ .setMaxShardsPerNode(numReplicas)).setPreemptiveCreateWindow("3HOUR")
+ .process(solrClient);
+
+ // cause some collections to be created
+ assertUpdateResponse(solrClient.add(alias,
+ sdoc("id","1","timestamp_dt", "2017-10-25T00:00:00Z")
--- End diff --
To reduce the overhead of the test, can we start one one collection, maybe two, before getting into pre-emption?
And this test should very likely be annotated with \@Slow
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r213400327
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -405,4 +435,54 @@ protected void doClose() {
collection, slice.getName(), DistributedUpdateProcessor.MAX_RETRIES_ON_FORWARD_DEAULT);
}
+
+ /**
+ * Create as many collections as required. This method loops to allow for the possibility that the routeTimestamp
+ * requires more than one collection to be created. Since multiple threads may be invoking maintain on separate
+ * requests to the same alias, we must pass in the name of the collection that this thread believes to be the most
+ * recent collection. This assumption is checked when the command is executed in the overseer. When this method
+ * finds that all collections required have been created it returns the (possibly new) most recent collection.
+ * The return value is ignored by the calling code in the async preemptive case.
+ *
+ * @param targetCollection the initial notion of the latest collection available.
+ * @param docTimestamp the timestamp from the document that determines routing
+ * @param printableId an identifier for the add command used in error messages
+ * @return The latest collection, including collections created during maintenance
+ */
+ public String maintain(String targetCollection, Instant docTimestamp, String printableId, boolean asyncSinglePassOnly) {
+ do { // typically we don't loop; it's only when we need to create a collection
+
+ // Note: This code no longer short circuits immediately when it sees that the expected latest
--- End diff --
That's fine, the comment mostly aimed at making the review process clearer. You're right it probably doesn't need to be carried forward
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r213419315
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -392,29 +393,73 @@ public void testPreemptiveCreation() throws Exception {
CollectionAdminRequest.setAliasProperty(alias)
.addProperty(TimeRoutedAlias.ROUTER_PREEMPTIVE_CREATE_MATH, "3DAY").process(solrClient);
- Thread.sleep(1000); // a moment to be sure the alias change has taken effect
-
assertUpdateResponse(add(alias, Collections.singletonList(
sdoc("id", "7", "timestamp_dt", "2017-10-25T23:01:00Z")), // should cause preemptive creation now
params));
assertUpdateResponse(solrClient.commit(alias));
waitCol("2017-10-27", numShards);
- waitCol("2017-10-28", numShards);
cols = new CollectionAdminRequest.ListAliases().process(solrClient).getAliasesAsLists().get(alias);
- assertEquals(6,cols.size());
+ assertEquals(5,cols.size()); // only one created in async case
assertNumDocs("2017-10-23", 1);
assertNumDocs("2017-10-24", 1);
assertNumDocs("2017-10-25", 5);
assertNumDocs("2017-10-26", 0);
assertNumDocs("2017-10-27", 0);
+
+ assertUpdateResponse(add(alias, Collections.singletonList(
+ sdoc("id", "8", "timestamp_dt", "2017-10-25T23:01:00Z")), // should cause preemptive creation now
+ params));
+ assertUpdateResponse(solrClient.commit(alias));
+ waitCol("2017-10-27", numShards);
+ waitCol("2017-10-28", numShards);
+
+ cols = new CollectionAdminRequest.ListAliases().process(solrClient).getAliasesAsLists().get(alias);
+ assertEquals(6,cols.size()); // Subsequent documents continue to create up to limit
+ assertNumDocs("2017-10-23", 1);
+ assertNumDocs("2017-10-24", 1);
+ assertNumDocs("2017-10-25", 6);
+ assertNumDocs("2017-10-26", 0);
+ assertNumDocs("2017-10-27", 0);
assertNumDocs("2017-10-28", 0);
QueryResponse resp;
resp = solrClient.query(alias, params(
"q", "*:*",
"rows", "10"));
- assertEquals(7, resp.getResults().getNumFound());
+ assertEquals(8, resp.getResults().getNumFound());
+
+ assertUpdateResponse(add(alias, Arrays.asList(
--- End diff --
addDocsAndCommit contains a lot of logic I don't really want, especially the shuffling of the input documents!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208448157
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -167,66 +173,121 @@ 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);
+ String candidateCollection = findCandidateCollectionGivenTimestamp(docTimestampToRoute, cmd.getPrintableId());
+ String targetCollection = createCollectionsIfRequired(docTimestampToRoute, candidateCollection, cmd.getPrintableId());
+ 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);
+ cmdDistrib.distribAdd(cmd, Collections.singletonList(targetLeaderNode), new ModifiableSolrParams(outParamsToLeader));
+ }
+ }
- if (targetCollection == null) {
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
- "Doc " + cmd.getPrintableId() + " couldn't be routed with " + timeRoutedAlias.getRouteField() + "=" + routeTimestamp);
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId);
+ case ASYNC_PREEMPTIVE:
+ // Note: creating an executor is slightly expensive, but only likely to happen once per hour/day/week
+ // (depending on time slice size for the TRA). Executor is used to ensure we pick up the MDC logging stuff
+ // from ExecutorUtil. Even though it is possible that multiple requests hit this code in the 1-2 sec that
+ // it takes to create a collection, it's an established anti-pattern to feed data with a very large number
+ // of client connections. This in mind, we only guard against spamming the overseer within a batch of
+ // updates, intentionally tolerating a low level of redundant requests in favor of simpler code. Most
+ // super-sized installations with many update clients will likely be multi-tenant and multiple tenants
+ // probably don't write to the same alias. As such, we have deferred any solution 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.
+ synchronized (execLock) {
+ if (preemptiveCreationExecutor == null) {
+ preemptiveCreationExecutor = preemptiveCreationExecutor();
+ }
+ preemptiveCreationExecutor.execute(() -> {
+ maintain(targetCollection, docTimestamp, printableId);
+ preemptiveCreationExecutor = null;
+ });
+ preemptiveCreationExecutor.shutdown(); // shutdown immediately to ensure no new requests accepted
+ }
+ return targetCollection;
+ case NONE:
+ return targetCollection; // just for clarity...
+ default:
+ return targetCollection; // could use fall through, but fall through is fiddly for later editors.
}
+ // do nothing if creationType == NONE
+ } catch (SolrException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
- // 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;
- }
+ /**
+ * Create an executor that can only handle one task at a time. Additional tasks are rejected silently.
+ * If we receive a batch update with hundreds of docs, that could queue up hundreds of calls to maintain().
+ * Such a situation will typically create the required collection on the first document and then uselessly
+ * spend time calculating that we don't need to create anything for subsequent documents. Therefore we simply
+ * want to silently discard any additional attempts to maintain this alias until the one in progress has completed.
+ */
+ private ExecutorService preemptiveCreationExecutor() {
- // 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());
- }
+ ThreadPoolExecutor.DiscardPolicy discardPolicy = new ThreadPoolExecutor.DiscardPolicy(); // exception never thrown
+ ArrayBlockingQueue<Runnable> oneAtATime = new ArrayBlockingQueue<>(1);
+ DefaultSolrThreadFactory threadFactory = new DefaultSolrThreadFactory("TRA-preemptive-creation");
- // Create a new collection?
- final Instant nextCollTimestamp = timeRoutedAlias.computeNextCollTimestamp(mostRecentCollTimestamp);
- if (routeTimestamp.isBefore(nextCollTimestamp)) {
- break; // thus we don't need another collection
- }
+ // Note: There is an interesting case that could crop up when the pre-create interval is longer than
+ // a single time slice in the alias. With that configuration it is possible that a doc that should
+ // cause several collections to be created is preceded by one that only creates a single collection. In that
+ // case we might be too conservative in our pre-creation, and only create one collection. Dealing with that
+ // presumably rare case adds complexity and is intentionally ignored at this time. If this shows itself to
+ // be a frequent or otherwise important use case this decision can be revisited.
- 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;
+ return newMDCAwareSingleThreadExecutor(threadFactory, discardPolicy, oneAtATime);
+ }
- if (thisCollection.equals(targetCollection)) {
- // pass on through; we've reached the right collection
- super.processAdd(cmd);
+ /**
+ * Determine if the a new collection will be required based on the document timestamp. Passing null for
+ * preemptiveCreateInterval tells you if the document is beyond all existing collections with a response of
+ * {@link CreationType#NONE} or {@link CreationType#SYNCHRONOUS}, and passing a valid date math for
+ * preemptiveCreateMath additionally distinguishes the case where the document is close enough to the end of
+ * the TRA to trigger preemptive creation but not beyond all existing collections with a value of
+ * {@link CreationType#ASYNC_PREEMPTIVE}.
+ *
+ * @param routeTimestamp The timestamp from the document
+ * @param preemptiveCreateMath The date math indicating the {@link TimeRoutedAlias#preemptiveCreateMath}
+ * @return a {@code CreationType} indicating if and how to create a collection
+ */
+ private CreationType requiresCreateCollection(Instant routeTimestamp, String preemptiveCreateMath) {
+ // Create a new collection?
+ final Instant mostRecentCollTimestamp = parsedCollectionsDesc.get(0).getKey();
+ final Instant nextCollTimestamp = timeRoutedAlias.computeNextCollTimestamp(mostRecentCollTimestamp);
+ if (isBlank(preemptiveCreateMath)) {
+ return !routeTimestamp.isBefore(nextCollTimestamp) ? SYNCHRONOUS : NONE;
} else {
- // send to the right collection
- SolrCmdDistributor.Node targetLeaderNode = routeDocToSlice(targetCollection, solrInputDocument);
- cmdDistrib.distribAdd(cmd, Collections.singletonList(targetLeaderNode), new ModifiableSolrParams(outParamsToLeader));
+ DateMathParser dateMathParser = new DateMathParser();
--- End diff --
Lets pull out a utility method to compute preemptNextColCreateTime so that the logic here isn't distracted by that & exception handling.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r213415931
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -322,6 +325,104 @@ public void testSliceRouting() throws Exception {
}
}
+ @Test
+ public void testPreemptiveCreation() throws Exception {
+ String configName = TimeRoutedAliasUpdateProcessorTest.configName + getTestName();
+ createConfigSet(configName);
+
+ final int numShards = 1 ;
+ final int numReplicas = 1 ;
+ CollectionAdminRequest.createTimeRoutedAlias(alias, "2017-10-23T00:00:00Z", "+1DAY", timeField,
+ CollectionAdminRequest.createCollection("_unused_", configName, numShards, numReplicas)
+ .setMaxShardsPerNode(numReplicas)).setPreemptiveCreateWindow("3HOUR")
+ .process(solrClient);
+
+ // cause some collections to be created
+ assertUpdateResponse(solrClient.add(alias,
+ sdoc("id","1","timestamp_dt", "2017-10-25T00:00:00Z")
+ ));
+ assertUpdateResponse(solrClient.commit(alias));
+
+ // wait for all the collections to exist...
+ waitCol("2017-10-23", numShards);
+ waitCol("2017-10-24", numShards);
+ waitCol("2017-10-25", numShards);
+
+ // normal update, nothing special, no collection creation required.
+ List<String> cols = new CollectionAdminRequest.ListAliases().process(solrClient).getAliasesAsLists().get(alias);
+ assertEquals(3,cols.size());
+
+ assertNumDocs("2017-10-23", 0);
+ assertNumDocs("2017-10-24", 0);
+ assertNumDocs("2017-10-25", 1);
+
+ // cause some collections to be created
+
+ ModifiableSolrParams params = params();
+ assertUpdateResponse(add(alias, Arrays.asList(
+ sdoc("id", "2", "timestamp_dt", "2017-10-24T00:00:00Z"),
+ sdoc("id", "3", "timestamp_dt", "2017-10-25T00:00:00Z"),
+ sdoc("id", "4", "timestamp_dt", "2017-10-23T00:00:00Z"),
+ sdoc("id", "5", "timestamp_dt", "2017-10-25T23:00:00Z")), // should cause preemptive creation
--- End diff --
I think this comment is obsolete, carried over from earlier versions?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212675378
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -405,4 +435,54 @@ protected void doClose() {
collection, slice.getName(), DistributedUpdateProcessor.MAX_RETRIES_ON_FORWARD_DEAULT);
}
+
+ /**
+ * Create as many collections as required. This method loops to allow for the possibility that the routeTimestamp
--- End diff --
Minor: `routeTimestamp` is now known as `docTimestamp` based on your edits.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208719459
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -343,8 +444,31 @@ public void testSliceRouting() throws Exception {
}
private void waitCol(final String datePart, int slices) {
- waitForState("waiting for collections to be created",alias + "_" + datePart,
- (liveNodes, collectionState) -> collectionState.getActiveSlices().size() == slices);
+ // collection to exist
+ String collection = alias + "_" + datePart;
+ waitForState("waiting for collections to be created", collection,
+ (liveNodes, collectionState) -> {
+ if (collectionState == null) {
+ // per predicate javadoc, this is what we get if the collection doesn't exist at all.
+ return false;
+ }
+ Collection<Slice> activeSlices = collectionState.getActiveSlices();
+ int size = activeSlices.size();
+ return size == slices;
+ });
+ // and alias to be aware of collection
+ long start = System.nanoTime(); // mumble mumble precommit mumble mumble...
+ while (!cluster.getSolrClient().getZkStateReader().getAliases().getCollectionAliasListMap().get(alias).contains(collection)) {
+ if (NANOSECONDS.toSeconds(System.nanoTime() - start) > 10) {
+ fail("took over 10 seconds after collection creation to update aliases");
+ } else {
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
--- End diff --
-- and return or throw. Interrupt means stop what you're doing.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r214120233
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -437,45 +461,46 @@ protected void doClose() {
/**
- * Create as many collections as required. This method loops to allow for the possibility that the routeTimestamp
+ * Create as many collections as required. This method loops to allow for the possibility that the docTimestamp
* requires more than one collection to be created. Since multiple threads may be invoking maintain on separate
* requests to the same alias, we must pass in the name of the collection that this thread believes to be the most
* recent collection. This assumption is checked when the command is executed in the overseer. When this method
* finds that all collections required have been created it returns the (possibly new) most recent collection.
* The return value is ignored by the calling code in the async preemptive case.
*
- * @param targetCollection the initial notion of the latest collection available.
* @param docTimestamp the timestamp from the document that determines routing
* @param printableId an identifier for the add command used in error messages
+ * @param targetCollectionDesc the descriptor for the presently selected collection which should also be
+ * the most recent collection in all cases where this method is invoked.
* @return The latest collection, including collections created during maintenance
*/
- public String maintain(String targetCollection, Instant docTimestamp, String printableId, boolean asyncSinglePassOnly) {
- do { // typically we don't loop; it's only when we need to create a collection
-
- // Note: This code no longer short circuits immediately when it sees that the expected latest
- // collection is the current latest collection. With the advent of preemptive collection creation
- // we always need to do the time based checks. Otherwise, we cannot handle the case where the
- // preemptive window is larger than our TRA's time slices
-
- // Check the doc isn't too far in the future
-
- if (NONE == requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow()))
- return targetCollection; // thus we don't need another collection
+ private String createAllRequiredCollections( Instant docTimestamp, String printableId,
+ Map.Entry<Instant, String> targetCollectionDesc) {
+ do {
+ switch(typeOfCreationRequired(docTimestamp, targetCollectionDesc.getKey())) {
+ case NONE:
+ return targetCollectionDesc.getValue(); // we don't need another collection
+ case ASYNC_PREEMPTIVE:
+ // can happen when preemptive interval is longer than one time slice
+ 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 unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212805827
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -94,13 +92,15 @@
private final SolrCmdDistributor cmdDistrib;
private final CollectionsHandler collHandler;
private final SolrParams outParamsToLeader;
+ @SuppressWarnings("FieldCanBeLocal")
private final CloudDescriptor cloudDesc;
private List<Map.Entry<Instant, String>> parsedCollectionsDesc; // k=timestamp (start), v=collection. Sorted descending
private Aliases parsedCollectionsAliases; // a cached reference to the source of what we parse into parsedCollectionsDesc
private SolrQueryRequest req;
+ private ExecutorService preemptiveCreationExecutor;
--- End diff --
No that's a good point
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212802626
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -94,13 +92,15 @@
private final SolrCmdDistributor cmdDistrib;
private final CollectionsHandler collHandler;
private final SolrParams outParamsToLeader;
+ @SuppressWarnings("FieldCanBeLocal")
private final CloudDescriptor cloudDesc;
private List<Map.Entry<Instant, String>> parsedCollectionsDesc; // k=timestamp (start), v=collection. Sorted descending
private Aliases parsedCollectionsAliases; // a cached reference to the source of what we parse into parsedCollectionsDesc
private SolrQueryRequest req;
+ private ExecutorService preemptiveCreationExecutor;
--- End diff --
Since it will be nulled out in another thread, we ought to declare this as volatile. I know this is being a bit pedantic since I don't think it'd be a real problem.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r208445857
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -167,66 +173,121 @@ 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);
+ String candidateCollection = findCandidateCollectionGivenTimestamp(docTimestampToRoute, cmd.getPrintableId());
+ String targetCollection = createCollectionsIfRequired(docTimestampToRoute, candidateCollection, cmd.getPrintableId());
+ 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);
+ cmdDistrib.distribAdd(cmd, Collections.singletonList(targetLeaderNode), new ModifiableSolrParams(outParamsToLeader));
+ }
+ }
- if (targetCollection == null) {
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
- "Doc " + cmd.getPrintableId() + " couldn't be routed with " + timeRoutedAlias.getRouteField() + "=" + routeTimestamp);
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId);
+ case ASYNC_PREEMPTIVE:
+ // Note: creating an executor is slightly expensive, but only likely to happen once per hour/day/week
+ // (depending on time slice size for the TRA). Executor is used to ensure we pick up the MDC logging stuff
+ // from ExecutorUtil. Even though it is possible that multiple requests hit this code in the 1-2 sec that
+ // it takes to create a collection, it's an established anti-pattern to feed data with a very large number
+ // of client connections. This in mind, we only guard against spamming the overseer within a batch of
+ // updates, intentionally tolerating a low level of redundant requests in favor of simpler code. Most
+ // super-sized installations with many update clients will likely be multi-tenant and multiple tenants
+ // probably don't write to the same alias. As such, we have deferred any solution 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.
+ synchronized (execLock) {
--- End diff --
Why have a synchronize here? URPs are single threaded and what we're locking on is local to this instance.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #433: SOLR-12357 Premptive creation of collections ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/433#discussion_r212802993
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +188,95 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+
+ private String createCollectionsIfRequired(Instant docTimestamp, String targetCollection, String printableId) {
+ // Even though it is possible that multiple requests hit this code in the 1-2 sec that
+ // it takes to create a collection, it's an established anti-pattern to feed data with a very large number
+ // of client connections. This in mind, we only guard against spamming the overseer within a batch of
+ // updates. We are intentionally tolerating a low level of redundant requests in favor of simpler code. Most
+ // super-sized installations with many update clients will likely be multi-tenant and multiple tenants
+ // 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.
+ try {
+ CreationType creationType = requiresCreateCollection(docTimestamp, timeRoutedAlias.getPreemptiveCreateWindow());
+ switch (creationType) {
+ case SYNCHRONOUS:
+ // This next line blocks until all collections required by the current document have been created
+ return maintain(targetCollection, docTimestamp, printableId, false);
+ 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;
+ });
+ }
+ return targetCollection;
+ case NONE:
+ return targetCollection; // just for clarity...
+ default:
+ return targetCollection; // could use fall through, but fall through is fiddly for later editors.
+ }
+ // do nothing if creationType == NONE
+ } catch (SolrException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Determine if the a new collection will be required based on the document timestamp. Passing null for
+ * preemptiveCreateInterval tells you if the document is beyond all existing collections with a response of
+ * {@link CreationType#NONE} or {@link CreationType#SYNCHRONOUS}, and passing a valid date math for
+ * preemptiveCreateMath additionally distinguishes the case where the document is close enough to the end of
+ * the TRA to trigger preemptive creation but not beyond all existing collections with a value of
+ * {@link CreationType#ASYNC_PREEMPTIVE}.
+ *
+ * @param routeTimestamp The timestamp from the document
+ * @param preemptiveCreateMath The date math indicating the {@link TimeRoutedAlias#preemptiveCreateMath}
+ * @return a {@code CreationType} indicating if and how to create a collection
+ */
+ private CreationType requiresCreateCollection(Instant routeTimestamp, String preemptiveCreateMath) {
+ final Instant mostRecentCollTimestamp = parsedCollectionsDesc.get(0).getKey();
+ final Instant nextCollTimestamp = timeRoutedAlias.computeNextCollTimestamp(mostRecentCollTimestamp);
+ if (!routeTimestamp.isBefore(nextCollTimestamp)) {
+ // current document is destined for a collection that doesn't exist, must create the destination
+ // to proceed with this add command
+ return SYNCHRONOUS;
+ }
+
+ if (isBlank(preemptiveCreateMath)) {
--- End diff --
nitpick: this method might flow better if you check for _not_ blank, then see if should return ASYNC_PREEMPTIVE, and at the very end return NONE. This would mean exactly 3 "return" statements of each type, which reads better IMO.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org