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/07/19 03:32:54 UTC
[GitHub] lucene-solr pull request #422: SOLR-12521 Premptive creation of collections ...
GitHub user nsoft opened a pull request:
https://github.com/apache/lucene-solr/pull/422
SOLR-12521 Premptive creation of collections in Time Routed Aliases
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nsoft/lucene-solr SOLR-12357
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucene-solr/pull/422.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 #422
----
commit cb8845336c9cfef6d587d6e605a02a138c124384
Author: Gus Heck <gu...@...>
Date: 2018-07-19T03:29:14Z
SOLR-12521
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204404955
--- Diff: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateAliasCmd.java ---
@@ -136,6 +139,16 @@ private Instant parseStart(String str, TimeZone zone) {
return start;
}
+ private void checkPreemptiveCreateWindow(String str) {
+ if (StringUtils.isNotBlank(str)) {
+ try {
+ new DateMathParser().parseMath( "-" + str);
--- End diff --
Woah, the "-" is subtle. If we want to auto-add a "-" then I think it should be done earlier immediately after reading the parameter in the TRA. I think that could work? Also, like auto delete age, shouldn't we be consistent and do the validation in the TRA constructor?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204520657
--- Diff: solr/solr-ref-guide/src/collections-api.adoc ---
@@ -597,6 +597,24 @@ without error. If there was no limit, than an erroneous value could trigger man
+
The default is 10 minutes.
+`router.preemptiveCreateWindow`::
+A date math expression that results in early creation of new collections.
--- End diff --
Add: "this date math expression must start with a number and not a +/-". I know you said similarly later but I think this is kinda important as it's not generic date math as found elsewhere in Solr.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204410513
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +219,59 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+ /**
+ * Create an executor that can only handle one task at a time. This is used to ensure that
+ */
+ private ThreadPoolExecutor singleThreadSingleTaskExecutor() {
+ ThreadPoolExecutor threadPoolExecutor = new ThreadPoolExecutor(1, 1, 1, TimeUnit.HOURS, new ArrayBlockingQueue<>(1));
--- End diff --
See `ExecutorUtil`; I think we are not allowed to create thread pools in Solr unless it's via those utils.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204516946
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -322,6 +327,59 @@ 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, new SolrInputDocument("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);
+
+ // cause some collections to be created
+
+ ModifiableSolrParams params = params();
+ assertUpdateResponse(add(alias, Arrays.asList(
+ sdoc("id", "2", "timestamp_dt", "2017-10-24T00:00:00Z"),
--- End diff --
Can you create one or two fewer collections, to thus reduce the overall weight of the test yet still test what we want?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204418038
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -170,55 +179,35 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
final Instant routeTimestamp = 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.");
+ String targetCollection = findCandidateCollectionGivenTimestamp(routeTimestamp, cmd.getPrintableId());
+ try {
+ String preemptiveCreateWindow = timeRoutedAlias.getPreemptiveCreateWindow();
+ if (StringUtils.isBlank(preemptiveCreateWindow) || // default: no pre-create
+ requiresCreateCollection(routeTimestamp, null)) { // or no collection exists for doc
+ targetCollection = new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(targetCollection); // then do it synchronously.
+ } else if (requiresCreateCollection(routeTimestamp, preemptiveCreateWindow )) { // else async...
+ String finalTargetCollection = targetCollection;
+ try {
+ asyncMaintainers.computeIfAbsent(timeRoutedAlias.getAliasName(),(alias) ->
+ singleThreadSingleTaskExecutor()).submit(() ->
+ new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(finalTargetCollection));
+ } catch (RejectedExecutionException e) {
+ // Note: There are some esoteric cases where the pre-create interval is a lot longer than
+ // a single time slice in the alias, and a doc that should cause several creations is blocked by
+ // one that only creates a single collection, but that requires some sort of long pause followed by a 2 doc
+ // burst and even then it only causes a synch create if there's a subsequent pause of more than another
+ // full time slice... These sorts of cases are probably more work than their value. If someone actually
+ // hits those cases frequently enough to notice, we can think about adding some further logic. For now,
+ // this is complicated enough as it is.
+ log.trace("Collection creation rejected, probably some other request has" +
--- End diff --
I'm not yet sure what to think of the scenario above, but note the code that existed before had an outer loop and that helped with edge cases by retrying.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204519114
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -322,6 +327,59 @@ 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, new SolrInputDocument("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);
+
+ // 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));
+
+ // given the long sleep below (without which the alias isn't available and we fail, make a request in the meantime
+ // targeting the new collection to ensure things don't blow up if docs come in while creation is in progress.
+ assertUpdateResponse(add(alias, Arrays.asList(
+ sdoc("id", "6", "timestamp_dt", "2017-10-25T23:01:00Z")), // should cause preemptive creation
+ params));
+ assertUpdateResponse(solrClient.commit(alias));
+
+ waitCol("2017-10-26", numShards); // this seems to only wait for the collection to exist, but
+ // not for the replicas to be created and the alias updated
+
+ // yuck, but collection creation seems to take 3sec on my machine doubling for slow, test loaded machines.
+ Thread.sleep(6000);
--- End diff --
yuck indeed. I propose that instead of waiting the (hopefully) maximum amount of time we think it might take, lets instead do a polling loop every second to see if the alias has the collection we want to see.
Also, lets add another document with a timestamp slightly after the one that triggered preemption, like: `2017-10-25T23:01:01Z` and observe it also goes where we want. This tests some additional pre-emptions in progress don't seem to cause errors or extra delays.
Also, lets test that the preemption doesn't block. This is rather important ;-) So in other words, after the document is added here in the test, we should be able to search for that document immediately. This is probably a one-liner assertion.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: SOLR-12357 Premptive creation of collections ...
Posted by fsparv <gi...@git.apache.org>.
Github user fsparv commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/422#discussion_r204746436
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +219,59 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+ /**
+ * Create an executor that can only handle one task at a time. This is used to ensure that
--- End diff --
oops, ensure that we don't spam the overseer with lots of MaintainRoutedAlias commands.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204417110
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -85,6 +93,7 @@
// Alternatively a Lock or CountDownLatch could have been used but they didn't seem
// to make it any easier.
private static ConcurrentHashMap<String, Semaphore> aliasToSemaphoreMap = new ConcurrentHashMap<>(4);
+ private static ConcurrentHashMap<String, ExecutorService> asyncMaintainers = new ConcurrentHashMap<>();
--- End diff --
Add comment please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204752840
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -170,55 +179,35 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
final Instant routeTimestamp = 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.");
+ String targetCollection = findCandidateCollectionGivenTimestamp(routeTimestamp, cmd.getPrintableId());
+ try {
+ String preemptiveCreateWindow = timeRoutedAlias.getPreemptiveCreateWindow();
+ if (StringUtils.isBlank(preemptiveCreateWindow) || // default: no pre-create
--- End diff --
I tend to prefer this in the tolerant in what you accept spirit, but I can switch it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204411897
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +219,59 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+ /**
+ * Create an executor that can only handle one task at a time. This is used to ensure that
+ */
+ private ThreadPoolExecutor singleThreadSingleTaskExecutor() {
+ ThreadPoolExecutor threadPoolExecutor = new ThreadPoolExecutor(1, 1, 1, TimeUnit.HOURS, new ArrayBlockingQueue<>(1));
+ // add a hook to keep from leaking threads in tests.
+ this.req.getCore().addCloseHook(new CloseHook() {
+ @Override
+ public void preClose(SolrCore core) {
+ try {
+ threadPoolExecutor.shutdown();
+ threadPoolExecutor.awaitTermination(250, TimeUnit.MILLISECONDS);
+ threadPoolExecutor.shutdownNow();
+ } catch (InterruptedException e) {
+ log.error("Interrupted while closing executor for TRA update processor");
+ }
+ }
+
+ @Override
+ public void postClose(SolrCore core) { }
+ });
+ return threadPoolExecutor;
+ }
+
+ /**
+ * 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, and passing a
+ * valid date math for preemptiveCreateWindow tells you if the document is close enough to the end of the
+ * TRA to trigger preemptive creation.
+ *
+ * @param routeTimestamp The timestamp from the document
+ * @param preemptiveCreateWindow The date math indicating the {@link TimeRoutedAlias#preemptiveCreateWindow}
+ * @return true if a new collection would be required.
+ */
+ private boolean requiresCreateCollection(Instant routeTimestamp, String preemptiveCreateWindow) {
+ // Create a new collection?
+ final Instant mostRecentCollTimestamp = parsedCollectionsDesc.get(0).getKey();
+ final Instant nextCollTimestamp = timeRoutedAlias.computeNextCollTimestamp(mostRecentCollTimestamp);
+ Instant nextCollCreateTime = nextCollTimestamp;
+ if (StringUtils.isNotBlank(preemptiveCreateWindow)) {
+ DateMathParser dateMathParser = new DateMathParser();
+ dateMathParser.setNow(Date.from(nextCollTimestamp));
+ try {
+ nextCollCreateTime = dateMathParser.parseMath("-" + preemptiveCreateWindow).toInstant();
--- End diff --
there's that subtle '-' again
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204405794
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -170,55 +179,35 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
final Instant routeTimestamp = 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.");
+ String targetCollection = findCandidateCollectionGivenTimestamp(routeTimestamp, cmd.getPrintableId());
+ try {
+ String preemptiveCreateWindow = timeRoutedAlias.getPreemptiveCreateWindow();
+ if (StringUtils.isBlank(preemptiveCreateWindow) || // default: no pre-create
--- End diff --
an empty window will be null, not an empty string. I know this can be a matter of taste but I prefer to prevent an empty string from occurring in the first place, and thus "==null" is more natural to test the non-empty 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 #422: 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/422#discussion_r204532263
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -262,8 +304,14 @@ private boolean updateParsedCollectionAliases() {
return false;
}
- /** Given the route key, finds the collection. Returns null if too old to go in last one. */
- private String findTargetCollectionGivenTimestamp(Instant docTimestamp) {
+ /**
+ * Given the route key, finds the correct collection or returns the most recent collection if the doc
+ * is in the future. Future docs will potentially cause creation of a collection that does not yet exist
+ * or an error if they exceed the maxFutureMs setting.
+ *
+ * @throws SolrException if the doc is too old to be stored in the TRA
+ */
+ private String findCandidateCollectionGivenTimestamp(Instant docTimestamp, String id) {
--- End diff --
Okay I see you changed the semantics; it is now a candidate but wasn't before.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204762784
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -322,6 +327,59 @@ 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, new SolrInputDocument("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);
+
+ // 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));
+
+ // given the long sleep below (without which the alias isn't available and we fail, make a request in the meantime
+ // targeting the new collection to ensure things don't blow up if docs come in while creation is in progress.
+ assertUpdateResponse(add(alias, Arrays.asList(
+ sdoc("id", "6", "timestamp_dt", "2017-10-25T23:01:00Z")), // should cause preemptive creation
+ params));
+ assertUpdateResponse(solrClient.commit(alias));
+
+ waitCol("2017-10-26", numShards); // this seems to only wait for the collection to exist, but
+ // not for the replicas to be created and the alias updated
+
+ // yuck, but collection creation seems to take 3sec on my machine doubling for slow, test loaded machines.
+ Thread.sleep(6000);
--- End diff --
with a polling loop we'll still need to time it out... to be any different, it will mean that if we fail, we will hang things up polling for a much longer timeout, but I guess that's ok. I hate tests that take forever when they fail.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204757584
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -170,55 +179,35 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
final Instant routeTimestamp = 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.");
+ String targetCollection = findCandidateCollectionGivenTimestamp(routeTimestamp, cmd.getPrintableId());
+ try {
+ String preemptiveCreateWindow = timeRoutedAlias.getPreemptiveCreateWindow();
+ if (StringUtils.isBlank(preemptiveCreateWindow) || // default: no pre-create
+ requiresCreateCollection(routeTimestamp, null)) { // or no collection exists for doc
+ targetCollection = new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(targetCollection); // then do it synchronously.
+ } else if (requiresCreateCollection(routeTimestamp, preemptiveCreateWindow )) { // else async...
+ String finalTargetCollection = targetCollection;
+ try {
+ asyncMaintainers.computeIfAbsent(timeRoutedAlias.getAliasName(),(alias) ->
+ singleThreadSingleTaskExecutor()).submit(() ->
+ new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(finalTargetCollection));
+ } catch (RejectedExecutionException e) {
+ // Note: There are some esoteric cases where the pre-create interval is a lot longer than
--- End diff --
Yes I know your concept with semaphores doesn't send to the overseer too... :) that's the whole goal either way here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204818612
--- Diff: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateAliasCmd.java ---
@@ -136,6 +139,16 @@ private Instant parseStart(String str, TimeZone zone) {
return start;
}
+ private void checkPreemptiveCreateWindow(String str) {
+ if (StringUtils.isNotBlank(str)) {
+ try {
+ new DateMathParser().parseMath( "-" + str);
--- End diff --
I do like that we don't make the user specify it -- that's nice. I agree it'd be confusing to know when to use +/- if we made users specify it. It'd be nice if other parts of Solr had this convenience, like range facet gap. I'm just saying we should internally convert to full DateMath eagerly; otherwise it's this partial datemath type of string and thus it's inaccurate to even say, via the variable name, that it's date math. Or... maybe we allow DateMath to have the leading operator be optional defaulting to '+', and thus it's accurate to say that "1SEC" is date math?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204529806
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -405,4 +454,56 @@ protected void doClose() {
collection, slice.getName());
}
+ private class Maintainer {
+ private final Instant routeTimestamp;
+ private final String id;
+
+ public Maintainer(Instant routeTimestamp, String id) {
+ this.routeTimestamp = routeTimestamp;
+ this.id = id;
+ }
+
+ public String maintain(String targetCollection) {
+ do { // typically we don't loop; it's only when we need to create a collection
+
+ // 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 String mostRecentCollName = parsedCollectionsDesc.get(0).getValue();
+ if (!mostRecentCollName.equals(targetCollection)) {
+ return targetCollection;
+ }
+
+ // Check the doc isn't too far in the future
+ final Instant maxFutureTime = Instant.now().plusMillis(timeRoutedAlias.getMaxFutureMs());
--- End diff --
I think this check is only pertinent for synchronous creation?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204415474
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -262,8 +304,14 @@ private boolean updateParsedCollectionAliases() {
return false;
}
- /** Given the route key, finds the collection. Returns null if too old to go in last one. */
- private String findTargetCollectionGivenTimestamp(Instant docTimestamp) {
+ /**
+ * Given the route key, finds the correct collection or returns the most recent collection if the doc
+ * is in the future. Future docs will potentially cause creation of a collection that does not yet exist
+ * or an error if they exceed the maxFutureMs setting.
+ *
+ * @throws SolrException if the doc is too old to be stored in the TRA
+ */
+ private String findCandidateCollectionGivenTimestamp(Instant docTimestamp, String id) {
--- End diff --
Why the "candidate" terminology? That word, to me, suggests something that is tentative -- that the ultimate choice may be something different.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204764304
--- Diff: solr/core/src/java/org/apache/solr/cloud/api/collections/TimeRoutedAlias.java ---
@@ -126,6 +128,7 @@ public static String formatCollectionNameFromInstant(String aliasName, Instant t
private final String routeField;
private final String intervalMath; // ex: +1DAY
private final long maxFutureMs;
+ private final String preemptiveCreateWindow;
--- 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 #422: 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/422#discussion_r204819121
--- Diff: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java ---
@@ -322,6 +327,59 @@ 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, new SolrInputDocument("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);
+
+ // 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));
+
+ // given the long sleep below (without which the alias isn't available and we fail, make a request in the meantime
+ // targeting the new collection to ensure things don't blow up if docs come in while creation is in progress.
+ assertUpdateResponse(add(alias, Arrays.asList(
+ sdoc("id", "6", "timestamp_dt", "2017-10-25T23:01:00Z")), // should cause preemptive creation
+ params));
+ assertUpdateResponse(solrClient.commit(alias));
+
+ waitCol("2017-10-26", numShards); // this seems to only wait for the collection to exist, but
+ // not for the replicas to be created and the alias updated
+
+ // yuck, but collection creation seems to take 3sec on my machine doubling for slow, test loaded machines.
+ Thread.sleep(6000);
--- End diff --
We needn't wait the full pre-emption window; we could wait as long as 10 seconds which ought to be plenty.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: SOLR-12357 Premptive creation of collections ...
Posted by fsparv <gi...@git.apache.org>.
Github user fsparv commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/422#discussion_r204751858
--- Diff: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateAliasCmd.java ---
@@ -136,6 +139,16 @@ private Instant parseStart(String str, TimeZone zone) {
return start;
}
+ private void checkPreemptiveCreateWindow(String str) {
+ if (StringUtils.isNotBlank(str)) {
+ try {
+ new DateMathParser().parseMath( "-" + str);
--- End diff --
I thought about allowing the user to specify the '-' but it just feels weird to think about. You wind up reading it like "Preemptively create a collection at -3 hours in the future relative to the last collection" or "Preemptively create a collection when it's needed -3 hours" rather than the much more natural thought/sentence "Preemptively create a collection 3 hours before it's needed." So I felt the API "read" better this way.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: SOLR-12357 Premptive creation of collections ...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft closed the pull request at:
https://github.com/apache/lucene-solr/pull/422
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204758735
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +219,59 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+ /**
+ * Create an executor that can only handle one task at a time. This is used to ensure that
+ */
+ private ThreadPoolExecutor singleThreadSingleTaskExecutor() {
+ ThreadPoolExecutor threadPoolExecutor = new ThreadPoolExecutor(1, 1, 1, TimeUnit.HOURS, new ArrayBlockingQueue<>(1));
--- End diff --
will look
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204536711
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -405,4 +454,56 @@ protected void doClose() {
collection, slice.getName());
}
+ private class Maintainer {
+ private final Instant routeTimestamp;
+ private final String id;
+
+ public Maintainer(Instant routeTimestamp, String id) {
+ this.routeTimestamp = routeTimestamp;
+ this.id = id;
+ }
+
+ public String maintain(String targetCollection) {
--- End diff --
I think the parameter name "targetCollection" is confusing. As I understand it, this is the assumed head collection name; it's more of a precondition. Maybe name the var "ifHeadCollName" (plus docs)? Perhaps rename Maintainer to NextCollectionMaker and it could simply be a Callable or Runnable or something like that even.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #422: SOLR-12357 Premptive creation of collections in Time...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on the issue:
https://github.com/apache/lucene-solr/pull/422
bleh didn't connect up because the wrong issue number got copied into the title. Closing will try again
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204761178
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -405,4 +454,56 @@ protected void doClose() {
collection, slice.getName());
}
+ private class Maintainer {
+ private final Instant routeTimestamp;
+ private final String id;
+
+ public Maintainer(Instant routeTimestamp, String id) {
+ this.routeTimestamp = routeTimestamp;
+ this.id = id;
+ }
+
+ public String maintain(String targetCollection) {
+ do { // typically we don't loop; it's only when we need to create a collection
+
+ // 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 String mostRecentCollName = parsedCollectionsDesc.get(0).getValue();
+ if (!mostRecentCollName.equals(targetCollection)) {
+ return targetCollection;
+ }
+
+ // Check the doc isn't too far in the future
+ final Instant maxFutureTime = Instant.now().plusMillis(timeRoutedAlias.getMaxFutureMs());
--- End diff --
The same code (this method) is executed for both sync and async cases see line 187.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204546734
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -170,55 +179,35 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
final Instant routeTimestamp = 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.");
+ String targetCollection = findCandidateCollectionGivenTimestamp(routeTimestamp, cmd.getPrintableId());
+ try {
+ String preemptiveCreateWindow = timeRoutedAlias.getPreemptiveCreateWindow();
+ if (StringUtils.isBlank(preemptiveCreateWindow) || // default: no pre-create
+ requiresCreateCollection(routeTimestamp, null)) { // or no collection exists for doc
+ targetCollection = new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(targetCollection); // then do it synchronously.
+ } else if (requiresCreateCollection(routeTimestamp, preemptiveCreateWindow )) { // else async...
+ String finalTargetCollection = targetCollection;
+ try {
+ asyncMaintainers.computeIfAbsent(timeRoutedAlias.getAliasName(),(alias) ->
+ singleThreadSingleTaskExecutor()).submit(() ->
+ new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(finalTargetCollection));
+ } catch (RejectedExecutionException e) {
+ // Note: There are some esoteric cases where the pre-create interval is a lot longer than
--- End diff --
Ah. I think we simply don't care if it was rejected; pre-emption is best-effort. Your log statement is good.
Maybe we don't want an Executor that can possibly reject us? I imagine every doc within this window during the collection creation might incur an exception throw? Look at createCollectionAfter() -- see how it uses a semaphore does nothing if another thread already has the lock. That's what we'd ideally have here too, right? Perhaps instead of a separate Maintainer class, we don't have that but instead have createCollectionAfter take an async boolean? If async is true, the "else" side (lock is already held) can simply return. if async is true if it grabs the lock, then it can go submit the logic in an executor that _won't_ throw a rejected exception. The release() needs to be performed when the work is done and not before. WDYT?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #422: SOLR-12521 Premptive creation of collections in Time...
Posted by nsoft <gi...@git.apache.org>.
Github user nsoft commented on the issue:
https://github.com/apache/lucene-solr/pull/422
rebased to fix commit message
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204395067
--- Diff: solr/core/src/java/org/apache/solr/cloud/api/collections/TimeRoutedAlias.java ---
@@ -126,6 +128,7 @@ public static String formatCollectionNameFromInstant(String aliasName, Instant t
private final String routeField;
private final String intervalMath; // ex: +1DAY
private final long maxFutureMs;
+ private final String preemptiveCreateWindow;
--- End diff --
Lets use "Math" as a field name suffix to be consistent with other date math fields
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204410113
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -230,6 +219,59 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
}
}
+ /**
+ * Create an executor that can only handle one task at a time. This is used to ensure that
--- End diff --
ensure that... ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204861300
--- Diff: solr/core/src/java/org/apache/solr/cloud/api/collections/TimeRoutedAlias.java ---
@@ -126,6 +128,7 @@ public static String formatCollectionNameFromInstant(String aliasName, Instant t
private final String routeField;
private final String intervalMath; // ex: +1DAY
private final long maxFutureMs;
+ private final String preemptiveCreateWindow;
--- 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 #422: 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/422#discussion_r204756996
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -170,55 +179,35 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
final Instant routeTimestamp = 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.");
+ String targetCollection = findCandidateCollectionGivenTimestamp(routeTimestamp, cmd.getPrintableId());
+ try {
+ String preemptiveCreateWindow = timeRoutedAlias.getPreemptiveCreateWindow();
+ if (StringUtils.isBlank(preemptiveCreateWindow) || // default: no pre-create
+ requiresCreateCollection(routeTimestamp, null)) { // or no collection exists for doc
+ targetCollection = new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(targetCollection); // then do it synchronously.
+ } else if (requiresCreateCollection(routeTimestamp, preemptiveCreateWindow )) { // else async...
+ String finalTargetCollection = targetCollection;
+ try {
+ asyncMaintainers.computeIfAbsent(timeRoutedAlias.getAliasName(),(alias) ->
+ singleThreadSingleTaskExecutor()).submit(() ->
+ new Maintainer(routeTimestamp, cmd.getPrintableId()).maintain(finalTargetCollection));
+ } catch (RejectedExecutionException e) {
+ // Note: There are some esoteric cases where the pre-create interval is a lot longer than
--- End diff --
Yeah, repeated throws would probably be a resource drain. My initial impulse was to keep this simple. I think I originally had an async boolean floating around and started trying to set up something with semaphores but then I thought of this and it seemed way simpler. "just fire it off and if someone already got there just fail quietly here rather than making the overseer execute a noop" I'll fiddle with it and see if there's a way to keep it simple without allowing a procession of exception throws.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #422: 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/422#discussion_r204417499
--- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
@@ -170,55 +179,35 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
final Instant routeTimestamp = 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.");
+ String targetCollection = findCandidateCollectionGivenTimestamp(routeTimestamp, cmd.getPrintableId());
+ try {
--- End diff --
I think I like the overall code flow better -- like moving the loop into Maintain. Any way I need to spend more time looking at this code to see if we're overlooking something.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org