[GitHub] lucene-solr pull request #304: SOLR-11722

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
37 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
GitHub user nsoft opened a pull request:

    https://github.com/apache/lucene-solr/pull/304

    SOLR-11722

    patch #3 including docs, test SSL fix and precommit fixes

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/nsoft/lucene-solr SOLR-11722

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/lucene-solr/pull/304.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 #304
   
----
commit aadae0c0e03bff019eac819919430b7c6eb7aabd
Author: Gus Heck <gus@...>
Date:   2018-01-11T20:09:25Z

    SOLR-11722 patch #3 including docs, test SSL fix and precommit fixes

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161070053
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
    @@ -252,6 +256,7 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
       /** Computes the timestamp of the next collection given the timestamp of the one before. */
       public static Instant computeNextCollTimestamp(Instant fromTimestamp, String intervalDateMath, TimeZone intervalTimeZone) {
         //TODO overload DateMathParser.parseMath to take tz and "now"
    +    // GH: I don't think that's necessary, you can set the TZ on now when you pass it in?
    --- End diff --
   
    True that it's not necessary; it's just a proposed convenience.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161068781
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java ---
    @@ -476,6 +451,31 @@ private static void addStatusToResponse(NamedList<Object> results, RequestStatus
           SolrIdentifierValidator.validateAliasName(req.getParams().get(NAME));
           return req.getParams().required().getAll(null, NAME, "collections");
         }),
    +    CREATEROUTEDALIAS_OP(CREATEROUTEDALIAS, (req, rsp, h) -> {
    +      String alias = req.getParams().get(NAME);
    +      SolrIdentifierValidator.validateAliasName(alias);
    +      Map<String, Object> params = req.getParams().required()
    +          .getAll(null, REQUIRED_ROUTING_PARAMS.toArray(new String[REQUIRED_ROUTING_PARAMS.size()]));
    +      req.getParams().getAll(params, NONREQUIRED_ROUTING_PARAMS);
    +      // subset the params to reuse the collection creation/parsing code
    +      ModifiableSolrParams collectionParams = extractPrefixedParams("create-collection.", req.getParams());
    +      if (collectionParams.get(NAME) != null) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "routed aliases calculate names for their " +
    +            "dependent collections, you cannot specify the name.");
    +        log.error("Could not create routed alias",solrException);
    --- End diff --
   
    I believe the general practice for BAD_REQUEST param validation is for Solr to not log it.  Besides, it's redundant with throwing the exception, and any exception thrown could be chosen to log or not centrally at some entrypoint juncture or by a thread handler for uncaught exceptions.  So lets not litter log statements that are superfluous unless there's some interesting/useful context.



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161070349
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessor.java ---
    @@ -99,6 +101,8 @@
           .parseDefaulting(ChronoField.HOUR_OF_DAY, 0)
           .parseDefaulting(ChronoField.MINUTE_OF_HOUR, 0)
           .parseDefaulting(ChronoField.SECOND_OF_MINUTE, 0)
    +      // Setting a timezone here is fine as a default, but generally need to clone with user's timezone, so that
    +      // truncation of milliseconds is consistent.
    --- End diff --
   
    As I indicated in a previous comment on JIRA, this comment isn't true.  We deliberately make the formatted collection name *insensitive* to the timezone.  Heck we ought to say that here.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161079495
 
    --- Diff: solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java ---
    @@ -0,0 +1,351 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.solr.cloud;
    +
    +import java.io.ByteArrayOutputStream;
    +import java.io.IOException;
    +import java.nio.charset.StandardCharsets;
    +import java.time.Instant;
    +import java.time.format.DateTimeFormatter;
    +import java.time.temporal.ChronoUnit;
    +import java.util.Date;
    +import java.util.Locale;
    +import java.util.Map;
    +
    +import com.fasterxml.jackson.core.type.TypeReference;
    +import com.fasterxml.jackson.databind.ObjectMapper;
    +import org.apache.http.HttpEntity;
    +import org.apache.http.client.methods.CloseableHttpResponse;
    +import org.apache.http.client.methods.HttpGet;
    +import org.apache.http.client.methods.HttpPost;
    +import org.apache.http.entity.ContentType;
    +import org.apache.http.entity.InputStreamEntity;
    +import org.apache.http.impl.client.CloseableHttpClient;
    +import org.apache.http.impl.client.HttpClients;
    +import org.apache.lucene.util.IOUtils;
    +import org.apache.solr.SolrTestCaseJ4;
    +import org.apache.solr.client.solrj.impl.CloudSolrClient;
    +import org.apache.solr.client.solrj.request.CollectionAdminRequest;
    +import org.apache.solr.common.cloud.Aliases;
    +import org.apache.solr.common.cloud.ZkStateReader;
    +import org.apache.solr.update.processor.TimeRoutedAliasUpdateProcessor;
    +import org.apache.solr.util.DateMathParser;
    +import org.junit.After;
    +import org.junit.AfterClass;
    +import org.junit.Before;
    +import org.junit.BeforeClass;
    +import org.junit.Test;
    +
    +/**
    + * Direct http tests of the CreateRoutedAlias functionality.
    + */
    +public class CreateRoutedAliasTest extends SolrCloudTestCase {
    --- End diff --
   
    Hmm; it'd be nice to combine this with the existing test to just have one TimeRoutedAliases test.  It can wait.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161074912
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -45,13 +147,92 @@ public CreateAliasCmd(OverseerCollectionMessageHandler ocmh) {
       public void call(ClusterState state, ZkNodeProps message, NamedList results)
           throws Exception {
         final String aliasName = message.getStr(NAME);
    -    final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    -    final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    -
         ZkStateReader zkStateReader = ocmh.zkStateReader;
    -    validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +    ZkStateReader.AliasesManager holder = zkStateReader.aliasesHolder;
    +    if (!anyRoutingParams(message)) {
    +      final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    +      final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    +      validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +      holder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +    } else {
    +      final String routedField = message.getStr(ROUTING_FIELD);
    +      final String routingType = message.getStr(ROUTING_TYPE);
    +      final String tz = message.getStr(TZ);
    +      final String start = message.getStr(START);
    +      final String increment = message.getStr(ROUTING_INCREMENT);
    +      final String maxFutureMs = message.getStr(ROUTING_MAX_FUTURE);
    +
    +      try {
    +        if (0 > Long.valueOf(maxFutureMs)) {
    +          throw new NumberFormatException("Negative value not allowed here");
    +        }
    +      } catch (NumberFormatException e) {
    +        throw new SolrException(BAD_REQUEST, ROUTING_MAX_FUTURE + " must be a valid long integer representing a number " +
    +            "of milliseconds greater than or equal to zero");
    +      }
     
    -    zkStateReader.aliasesHolder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +      // Validate we got everything we need
    +      if (routedField == null || routingType == null || start == null || increment == null) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "If any of " + CREATE_ROUTED_ALIAS_PARAMS +
    +            " are supplied, then all of " + REQUIRED_ROUTING_PARAMS + " must be present.");
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +      }
    +
    +      if (!"time".equals(routingType)) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "Only time based routing is supported at this time");
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +      }
    +      // Check for invalid timezone
    +      if(tz != null && !TimeZoneUtils.KNOWN_TIMEZONE_IDS.contains(tz)) {
    --- End diff --
   
    You needn't explicitly validate TZ here; call TimeZoneUtils.parseTimezone instead which does so (which I added recently).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161076165
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -45,13 +147,92 @@ public CreateAliasCmd(OverseerCollectionMessageHandler ocmh) {
       public void call(ClusterState state, ZkNodeProps message, NamedList results)
           throws Exception {
         final String aliasName = message.getStr(NAME);
    -    final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    -    final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    -
         ZkStateReader zkStateReader = ocmh.zkStateReader;
    -    validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +    ZkStateReader.AliasesManager holder = zkStateReader.aliasesHolder;
    +    if (!anyRoutingParams(message)) {
    +      final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    +      final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    +      validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +      holder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +    } else {
    +      final String routedField = message.getStr(ROUTING_FIELD);
    +      final String routingType = message.getStr(ROUTING_TYPE);
    +      final String tz = message.getStr(TZ);
    +      final String start = message.getStr(START);
    +      final String increment = message.getStr(ROUTING_INCREMENT);
    +      final String maxFutureMs = message.getStr(ROUTING_MAX_FUTURE);
    +
    +      try {
    +        if (0 > Long.valueOf(maxFutureMs)) {
    +          throw new NumberFormatException("Negative value not allowed here");
    +        }
    +      } catch (NumberFormatException e) {
    +        throw new SolrException(BAD_REQUEST, ROUTING_MAX_FUTURE + " must be a valid long integer representing a number " +
    +            "of milliseconds greater than or equal to zero");
    +      }
     
    -    zkStateReader.aliasesHolder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +      // Validate we got everything we need
    +      if (routedField == null || routingType == null || start == null || increment == null) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "If any of " + CREATE_ROUTED_ALIAS_PARAMS +
    +            " are supplied, then all of " + REQUIRED_ROUTING_PARAMS + " must be present.");
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +      }
    +
    +      if (!"time".equals(routingType)) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "Only time based routing is supported at this time");
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +      }
    +      // Check for invalid timezone
    +      if(tz != null && !TimeZoneUtils.KNOWN_TIMEZONE_IDS.contains(tz)) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "Invalid timezone:" + tz);
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +
    +      }
    +      TimeZone zone;
    +      if (tz != null) {
    +        zone = TimeZoneUtils.getTimeZone(tz);
    +      } else {
    +        zone = TimeZoneUtils.getTimeZone("UTC");
    +      }
    +      DateTimeFormatter fmt = DATE_TIME_FORMATTER.withZone(zone.toZoneId());
    +
    +      // check that the increment is valid date math
    +      String checkIncrement = ISO_INSTANT.format(Instant.now()) + increment;
    +      DateMathParser.parseMath(new Date(), checkIncrement); // exception if invalid increment
    --- End diff --
   
    Maybe I wasn't clear before.  I mean create a new DateMathParser using the constructor that accept the timezone (and we can supply ours; null is okay).  Then call the instance method `parseMath(str)` on it (not the static with 'now').  the arg should be the start metadata.  The "now" will default to the current instant in time which is what we want.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161077363
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -45,13 +147,92 @@ public CreateAliasCmd(OverseerCollectionMessageHandler ocmh) {
       public void call(ClusterState state, ZkNodeProps message, NamedList results)
           throws Exception {
         final String aliasName = message.getStr(NAME);
    -    final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    -    final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    -
         ZkStateReader zkStateReader = ocmh.zkStateReader;
    -    validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +    ZkStateReader.AliasesManager holder = zkStateReader.aliasesHolder;
    +    if (!anyRoutingParams(message)) {
    +      final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    +      final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    +      validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +      holder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +    } else {
    +      final String routedField = message.getStr(ROUTING_FIELD);
    +      final String routingType = message.getStr(ROUTING_TYPE);
    +      final String tz = message.getStr(TZ);
    +      final String start = message.getStr(START);
    +      final String increment = message.getStr(ROUTING_INCREMENT);
    +      final String maxFutureMs = message.getStr(ROUTING_MAX_FUTURE);
    +
    +      try {
    +        if (0 > Long.valueOf(maxFutureMs)) {
    +          throw new NumberFormatException("Negative value not allowed here");
    +        }
    +      } catch (NumberFormatException e) {
    +        throw new SolrException(BAD_REQUEST, ROUTING_MAX_FUTURE + " must be a valid long integer representing a number " +
    +            "of milliseconds greater than or equal to zero");
    +      }
     
    -    zkStateReader.aliasesHolder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +      // Validate we got everything we need
    +      if (routedField == null || routingType == null || start == null || increment == null) {
    --- End diff --
   
    I think much of these param validation checks are supposed to be handled at the CollectionsHandler level; no?  I'm not 100% sure.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161071581
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java ---
    @@ -962,6 +982,76 @@ public static CollectionOperation get(CollectionAction action) {
         }
       }
     
    +  private static SolrParams convertToV1WhenRequired(SolrQueryRequest req, ModifiableSolrParams params) {
    +    SolrParams v1Params = params; // (maybe...)
    +
    +    // in the v2 world we get a data map based on the json request, from the CommandOperation associated
    +    // with the request, so locate that if we can.. if we find it we have to translate the v2 request
    +    // properties to v1 params, otherwise we're already good to go.
    +    List<CommandOperation> cmds = req.getCommands(true);
    +    if (cmds.size() > 1) {
    +      // todo: not sure if this is the right thing to do here, but also not sure what to do if there is more than one...
    +      throw new SolrException(BAD_REQUEST, "Only one command is allowed when creating a routed alias");
    +    }
    +    CommandOperation c = cmds.size() == 0 ? null : cmds.get(0);
    +    if (c != null) {  // v2 api, do conversion to v1
    +      v1Params = new BaseHandlerApiSupport.V2ToV1SolrParams(CollectionApiMapping.Meta.CREATE_COLLECTION,
    +              req.getPathTemplateValues(), true, params,
    +              new CommandOperation("create", c.getDataMap().get("create-collection")));
    +    }
    +    return v1Params;
    +  }
    +
    +  private static Map<String, Object> parseColletionCreationProps(CollectionsHandler h, SolrParams params, String prefix)
    --- End diff --
   
    typo


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161071815
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -30,13 +44,101 @@
     import org.apache.solr.common.cloud.ZkStateReader;
     import org.apache.solr.common.util.NamedList;
     import org.apache.solr.common.util.StrUtils;
    +import org.apache.solr.update.processor.TimeRoutedAliasUpdateProcessor;
    +import org.apache.solr.util.DateMathParser;
    +import org.apache.solr.util.TimeZoneUtils;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
     
    +import static java.time.format.DateTimeFormatter.ISO_INSTANT;
    +import static org.apache.solr.cloud.OverseerCollectionMessageHandler.COLL_CONF;
    +import static org.apache.solr.common.SolrException.ErrorCode.BAD_REQUEST;
     import static org.apache.solr.common.params.CommonParams.NAME;
    +import static org.apache.solr.common.params.CommonParams.TZ;
    +import static org.apache.solr.handler.admin.CollectionsHandler.ROUTED_ALIAS_COLLECTION_PROP_PFX;
    +import static org.apache.solr.update.processor.TimeRoutedAliasUpdateProcessor.DATE_TIME_FORMATTER;
     
     
     public class CreateAliasCmd implements Cmd {
    +
    +  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
    +
    +  public static final String ROUTING_TYPE = "router.name";
    +  public static final String ROUTING_FIELD = "router.field";
    +  public static final String ROUTING_INCREMENT = "router.interval";
    +  public static final String ROUTING_MAX_FUTURE = "router.max-future-ms";
    +  public static final String START = "start";
    +  // Collection constants should all reflect names in the v2 structured input for this command, not v1
    +  // names used for CREATE
    +  public static final String CREATE_COLLECTION_CONFIG = "create-collection.config";
    --- End diff --
   
    Ugh, all those CREATE_COLLECTION_* constants are a maintenance burden; surely we can avoid them?  I really want to avoid double-specificity of the collection-creation stuff in any shape or form (not in docs/constants/etc.)


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161071261
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java ---
    @@ -218,6 +218,7 @@ public OverseerCollectionMessageHandler(ZkStateReader zkStateReader, String myId
             .put(RELOAD, this::reloadCollection)
             .put(DELETE, new DeleteCollectionCmd(this))
             .put(CREATEALIAS, new CreateAliasCmd(this))
    +        .put(CREATEROUTEDALIAS, new CreateAliasCmd(this))
    --- End diff --
   
    Hmm; is there a point to this?  I guess it's debatable.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161074624
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -45,13 +147,92 @@ public CreateAliasCmd(OverseerCollectionMessageHandler ocmh) {
       public void call(ClusterState state, ZkNodeProps message, NamedList results)
           throws Exception {
         final String aliasName = message.getStr(NAME);
    -    final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    -    final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    -
         ZkStateReader zkStateReader = ocmh.zkStateReader;
    -    validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +    ZkStateReader.AliasesManager holder = zkStateReader.aliasesHolder;
    +    if (!anyRoutingParams(message)) {
    +      final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    +      final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    +      validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +      holder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +    } else {
    +      final String routedField = message.getStr(ROUTING_FIELD);
    +      final String routingType = message.getStr(ROUTING_TYPE);
    +      final String tz = message.getStr(TZ);
    +      final String start = message.getStr(START);
    +      final String increment = message.getStr(ROUTING_INCREMENT);
    +      final String maxFutureMs = message.getStr(ROUTING_MAX_FUTURE);
    +
    +      try {
    +        if (0 > Long.valueOf(maxFutureMs)) {
    --- End diff --
   
    use parseLong


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161079147
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -68,34 +249,100 @@ public void call(ClusterState state, ZkNodeProps message, NamedList results)
         Thread.sleep(100);
       }
     
    +  private Map<String, String> buildAliasMap(String routedField, String routingType, String tz, String increment, String maxFutureMs, ZkNodeProps collectionProps) {
    +    Map<String, Object> properties = collectionProps.getProperties();
    +    Map<String,String> cleanMap = properties.entrySet().stream()
    +        .filter(stringObjectEntry ->
    +            !"fromApi".equals(stringObjectEntry.getKey())
    +                && !"stateFormat".equals(stringObjectEntry.getKey())
    +                && !"name".equals(stringObjectEntry.getKey()))
    +        .collect(Collectors.toMap((e) -> "collection-create." + e.getKey(), e -> String.valueOf(e.getValue())));
    +    cleanMap.put(ROUTING_FIELD, routedField);
    +    cleanMap.put(ROUTING_TYPE, routingType);
    +    cleanMap.put(ROUTING_INCREMENT, increment);
    +    cleanMap.put(ROUTING_MAX_FUTURE, maxFutureMs);
    +    cleanMap.put(TZ, tz);
    +    return cleanMap;
    +  }
    +
    +  private Instant validateStart(TimeZone zone, DateTimeFormatter fmt, String start) {
    +    // This is the normal/easy case, if we can get away with this great!
    +    TemporalAccessor startTime = attemptTimeStampParsing(start, zone.toZoneId());
    +    if (startTime == null) {
    +      // No luck, they gave us either date math, or garbage, so we have to do more work to figure out which and
    +      // to make sure it's valid date math and that it doesn't encode any millisecond precision.
    +      ZonedDateTime now = ZonedDateTime.now(zone.toZoneId()).truncatedTo(ChronoUnit.MILLIS);
    +      try {
    +        Date date = DateMathParser.parseMath(Date.from(now.toInstant()), start);
    +        String reformatted = fmt.format(date.toInstant().truncatedTo(ChronoUnit.MILLIS));
    +        Date reparse = Date.from(Instant.from(DATE_TIME_FORMATTER.parse(reformatted)));
    +        if (!reparse.equals(date)) {
    +          throw new SolrException(BAD_REQUEST,
    +              "Formatted time did not have the same milliseconds as original: " + date.getTime() + " vs. " +
    +                  reparse.getTime() + " This indicates that you used date math that includes milliseconds. " +
    +                  "(Hint: 'NOW' used without rounding always has this problem)" );
    +        }
    +        return date.toInstant();
    +      } catch (SolrException e) {
    +        throw new SolrException(BAD_REQUEST,
    +            "Start Time for the first collection must be a timestamp of the format yyyy-MM-dd_HH_mm_ss, " +
    +                "or a valid date math expression not containing specific milliseconds", e);
    +      }
    +    }
    +    return Instant.from(startTime);
    +  }
    +
    +  private TemporalAccessor attemptTimeStampParsing(String start, ZoneId zone) {
    +    try {
    +      DATE_TIME_FORMATTER.withZone(zone);
    +      return DATE_TIME_FORMATTER.parse(start);
    +    } catch (DateTimeParseException e) {
    +      return null;
    +    }
    +  }
    +
    +  private boolean anyRoutingParams(ZkNodeProps message) {
    +
    +    return message.containsKey(ROUTING_FIELD) || message.containsKey(ROUTING_TYPE) || message.containsKey(START)
    +        || message.containsKey(ROUTING_INCREMENT) || message.containsKey(TZ);
    +  }
    +
       private void validateAllCollectionsExistAndNoDups(List<String> collectionList, ZkStateReader zkStateReader) {
         final String collectionStr = StrUtils.join(collectionList, ',');
     
         if (new HashSet<>(collectionList).size() != collectionList.size()) {
    -      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
    +      throw new SolrException(BAD_REQUEST,
               String.format(Locale.ROOT,  "Can't create collection alias for collections='%s', since it contains duplicates", collectionStr));
         }
         ClusterState clusterState = zkStateReader.getClusterState();
         Set<String> aliasNames = zkStateReader.getAliases().getCollectionAliasListMap().keySet();
         for (String collection : collectionList) {
           if (clusterState.getCollectionOrNull(collection) == null && !aliasNames.contains(collection)) {
    -        throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
    +        throw new SolrException(BAD_REQUEST,
                 String.format(Locale.ROOT,  "Can't create collection alias for collections='%s', '%s' is not an existing collection or alias", collectionStr, collection));
           }
         }
       }
    -  
    +
       /**
        * The v2 API directs that the 'collections' parameter be provided as a JSON array (e.g. ["a", "b"]).  We also
        * maintain support for the legacy format, a comma-separated list (e.g. a,b).
        */
       @SuppressWarnings("unchecked")
       private List<String> parseCollectionsParameter(Object colls) {
    -    if (colls == null) throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "missing collections param");
    +    if (colls == null) throw new SolrException(BAD_REQUEST, "missing collections param");
         if (colls instanceof List) return (List<String>) colls;
         return StrUtils.splitSmart(colls.toString(), ",", true).stream()
             .map(String::trim)
             .collect(Collectors.toList());
       }
     
    +  private ZkNodeProps selectByPrefix(String prefix, ZkNodeProps source) {
    --- End diff --
   
    I think the use of the single element array as a holder should be avoided -- I mean I've done it in some circumstances but here you're only doing it to use Java 8 streams, which IMO isn't a good reason for that hack. Either use a standard loop construction, or use the version of Stream.collect that takes the supplier & accumulator to take your Java 8 kung-foo to the next level ;-)  'course most people reading code using such esoteric Java 8 stream features will need to read the docs to figure out what the heck is going on.  Trade-offs...



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161078056
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -68,34 +249,100 @@ public void call(ClusterState state, ZkNodeProps message, NamedList results)
         Thread.sleep(100);
       }
     
    +  private Map<String, String> buildAliasMap(String routedField, String routingType, String tz, String increment, String maxFutureMs, ZkNodeProps collectionProps) {
    +    Map<String, Object> properties = collectionProps.getProperties();
    +    Map<String,String> cleanMap = properties.entrySet().stream()
    +        .filter(stringObjectEntry ->
    +            !"fromApi".equals(stringObjectEntry.getKey())
    --- End diff --
   
    These 3 constants here are unfortunate.... maybe they could be in a constant Set and checked here?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161074349
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java ---
    @@ -476,6 +451,31 @@ private static void addStatusToResponse(NamedList<Object> results, RequestStatus
           SolrIdentifierValidator.validateAliasName(req.getParams().get(NAME));
           return req.getParams().required().getAll(null, NAME, "collections");
         }),
    +    CREATEROUTEDALIAS_OP(CREATEROUTEDALIAS, (req, rsp, h) -> {
    +      String alias = req.getParams().get(NAME);
    +      SolrIdentifierValidator.validateAliasName(alias);
    +      Map<String, Object> params = req.getParams().required()
    +          .getAll(null, REQUIRED_ROUTING_PARAMS.toArray(new String[REQUIRED_ROUTING_PARAMS.size()]));
    +      req.getParams().getAll(params, NONREQUIRED_ROUTING_PARAMS);
    +      // subset the params to reuse the collection creation/parsing code
    +      ModifiableSolrParams collectionParams = extractPrefixedParams("create-collection.", req.getParams());
    +      if (collectionParams.get(NAME) != null) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "routed aliases calculate names for their " +
    +            "dependent collections, you cannot specify the name.");
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +      }
    +      SolrParams v1Params = convertToV1WhenRequired(req, collectionParams);
    +
    +      // We need to add this temporary name just to pass validation.
    --- End diff --
   
    We need to insist that the collection config name is passed here.  It's very problematic to depend on automatic creation.  On a related note, when it's time to document TRA's, we may want to warn against data driven schema.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161078867
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -68,34 +249,100 @@ public void call(ClusterState state, ZkNodeProps message, NamedList results)
         Thread.sleep(100);
       }
     
    +  private Map<String, String> buildAliasMap(String routedField, String routingType, String tz, String increment, String maxFutureMs, ZkNodeProps collectionProps) {
    +    Map<String, Object> properties = collectionProps.getProperties();
    +    Map<String,String> cleanMap = properties.entrySet().stream()
    +        .filter(stringObjectEntry ->
    +            !"fromApi".equals(stringObjectEntry.getKey())
    +                && !"stateFormat".equals(stringObjectEntry.getKey())
    +                && !"name".equals(stringObjectEntry.getKey()))
    +        .collect(Collectors.toMap((e) -> "collection-create." + e.getKey(), e -> String.valueOf(e.getValue())));
    +    cleanMap.put(ROUTING_FIELD, routedField);
    +    cleanMap.put(ROUTING_TYPE, routingType);
    +    cleanMap.put(ROUTING_INCREMENT, increment);
    +    cleanMap.put(ROUTING_MAX_FUTURE, maxFutureMs);
    +    cleanMap.put(TZ, tz);
    +    return cleanMap;
    +  }
    +
    +  private Instant validateStart(TimeZone zone, DateTimeFormatter fmt, String start) {
    --- End diff --
   
    This appears redundant with using DateMathParser earlier and I'm sure we don't need so much code for this matter (ignore millisecond truncation; in practice the user is going to do something like `/MONTH` or `/DAY` for start.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161075027
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -45,13 +147,92 @@ public CreateAliasCmd(OverseerCollectionMessageHandler ocmh) {
       public void call(ClusterState state, ZkNodeProps message, NamedList results)
           throws Exception {
         final String aliasName = message.getStr(NAME);
    -    final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    -    final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    -
         ZkStateReader zkStateReader = ocmh.zkStateReader;
    -    validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +    ZkStateReader.AliasesManager holder = zkStateReader.aliasesHolder;
    +    if (!anyRoutingParams(message)) {
    +      final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    +      final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    +      validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +      holder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +    } else {
    +      final String routedField = message.getStr(ROUTING_FIELD);
    +      final String routingType = message.getStr(ROUTING_TYPE);
    +      final String tz = message.getStr(TZ);
    +      final String start = message.getStr(START);
    +      final String increment = message.getStr(ROUTING_INCREMENT);
    +      final String maxFutureMs = message.getStr(ROUTING_MAX_FUTURE);
    +
    +      try {
    +        if (0 > Long.valueOf(maxFutureMs)) {
    +          throw new NumberFormatException("Negative value not allowed here");
    +        }
    +      } catch (NumberFormatException e) {
    +        throw new SolrException(BAD_REQUEST, ROUTING_MAX_FUTURE + " must be a valid long integer representing a number " +
    +            "of milliseconds greater than or equal to zero");
    +      }
     
    -    zkStateReader.aliasesHolder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +      // Validate we got everything we need
    +      if (routedField == null || routingType == null || start == null || increment == null) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "If any of " + CREATE_ROUTED_ALIAS_PARAMS +
    +            " are supplied, then all of " + REQUIRED_ROUTING_PARAMS + " must be present.");
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +      }
    +
    +      if (!"time".equals(routingType)) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "Only time based routing is supported at this time");
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +      }
    +      // Check for invalid timezone
    +      if(tz != null && !TimeZoneUtils.KNOWN_TIMEZONE_IDS.contains(tz)) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "Invalid timezone:" + tz);
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +
    +      }
    +      TimeZone zone;
    +      if (tz != null) {
    +        zone = TimeZoneUtils.getTimeZone(tz);
    +      } else {
    +        zone = TimeZoneUtils.getTimeZone("UTC");
    +      }
    +      DateTimeFormatter fmt = DATE_TIME_FORMATTER.withZone(zone.toZoneId());
    --- End diff --
   
    No we don't want to use the TZ to format the collection name


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user fsparv commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161093191
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -30,13 +44,101 @@
     import org.apache.solr.common.cloud.ZkStateReader;
     import org.apache.solr.common.util.NamedList;
     import org.apache.solr.common.util.StrUtils;
    +import org.apache.solr.update.processor.TimeRoutedAliasUpdateProcessor;
    +import org.apache.solr.util.DateMathParser;
    +import org.apache.solr.util.TimeZoneUtils;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
     
    +import static java.time.format.DateTimeFormatter.ISO_INSTANT;
    +import static org.apache.solr.cloud.OverseerCollectionMessageHandler.COLL_CONF;
    +import static org.apache.solr.common.SolrException.ErrorCode.BAD_REQUEST;
     import static org.apache.solr.common.params.CommonParams.NAME;
    +import static org.apache.solr.common.params.CommonParams.TZ;
    +import static org.apache.solr.handler.admin.CollectionsHandler.ROUTED_ALIAS_COLLECTION_PROP_PFX;
    +import static org.apache.solr.update.processor.TimeRoutedAliasUpdateProcessor.DATE_TIME_FORMATTER;
     
     
     public class CreateAliasCmd implements Cmd {
    +
    +  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
    +
    +  public static final String ROUTING_TYPE = "router.name";
    +  public static final String ROUTING_FIELD = "router.field";
    +  public static final String ROUTING_INCREMENT = "router.interval";
    +  public static final String ROUTING_MAX_FUTURE = "router.max-future-ms";
    +  public static final String START = "start";
    +  // Collection constants should all reflect names in the v2 structured input for this command, not v1
    +  // names used for CREATE
    +  public static final String CREATE_COLLECTION_CONFIG = "create-collection.config";
    --- End diff --
   
    I agree, I don't like having them here and in the collection admin request  but solrj can't see core. Is there a constants class I can move them to that solrj can see?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user fsparv commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161096191
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -45,13 +147,92 @@ public CreateAliasCmd(OverseerCollectionMessageHandler ocmh) {
       public void call(ClusterState state, ZkNodeProps message, NamedList results)
           throws Exception {
         final String aliasName = message.getStr(NAME);
    -    final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    -    final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    -
         ZkStateReader zkStateReader = ocmh.zkStateReader;
    -    validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +    ZkStateReader.AliasesManager holder = zkStateReader.aliasesHolder;
    +    if (!anyRoutingParams(message)) {
    +      final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    +      final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    +      validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +      holder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +    } else {
    +      final String routedField = message.getStr(ROUTING_FIELD);
    +      final String routingType = message.getStr(ROUTING_TYPE);
    +      final String tz = message.getStr(TZ);
    +      final String start = message.getStr(START);
    +      final String increment = message.getStr(ROUTING_INCREMENT);
    +      final String maxFutureMs = message.getStr(ROUTING_MAX_FUTURE);
    +
    +      try {
    +        if (0 > Long.valueOf(maxFutureMs)) {
    +          throw new NumberFormatException("Negative value not allowed here");
    +        }
    +      } catch (NumberFormatException e) {
    +        throw new SolrException(BAD_REQUEST, ROUTING_MAX_FUTURE + " must be a valid long integer representing a number " +
    +            "of milliseconds greater than or equal to zero");
    +      }
     
    -    zkStateReader.aliasesHolder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +      // Validate we got everything we need
    +      if (routedField == null || routingType == null || start == null || increment == null) {
    --- End diff --
   
    in this area of the code I implement checks for the following:
    name
    start
    router.field
    router.name
    router.interval
    router.max-future-ms
    TZ
    collection-create.config
   
    Only that last one could possibly be left to the collection-create infrastructure. The rest are ours (note that the router ones are not the collection level routing but the alias level routing).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #304: SOLR-11722

barrotsteindev
In reply to this post by barrotsteindev
Github user fsparv commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/304#discussion_r161101824
 
    --- Diff: solr/core/src/java/org/apache/solr/cloud/CreateAliasCmd.java ---
    @@ -45,13 +147,92 @@ public CreateAliasCmd(OverseerCollectionMessageHandler ocmh) {
       public void call(ClusterState state, ZkNodeProps message, NamedList results)
           throws Exception {
         final String aliasName = message.getStr(NAME);
    -    final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    -    final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    -
         ZkStateReader zkStateReader = ocmh.zkStateReader;
    -    validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +    ZkStateReader.AliasesManager holder = zkStateReader.aliasesHolder;
    +    if (!anyRoutingParams(message)) {
    +      final List<String> canonicalCollectionList = parseCollectionsParameter(message.get("collections"));
    +      final String canonicalCollectionsString = StrUtils.join(canonicalCollectionList, ',');
    +      validateAllCollectionsExistAndNoDups(canonicalCollectionList, zkStateReader);
    +      holder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +    } else {
    +      final String routedField = message.getStr(ROUTING_FIELD);
    +      final String routingType = message.getStr(ROUTING_TYPE);
    +      final String tz = message.getStr(TZ);
    +      final String start = message.getStr(START);
    +      final String increment = message.getStr(ROUTING_INCREMENT);
    +      final String maxFutureMs = message.getStr(ROUTING_MAX_FUTURE);
    +
    +      try {
    +        if (0 > Long.valueOf(maxFutureMs)) {
    +          throw new NumberFormatException("Negative value not allowed here");
    +        }
    +      } catch (NumberFormatException e) {
    +        throw new SolrException(BAD_REQUEST, ROUTING_MAX_FUTURE + " must be a valid long integer representing a number " +
    +            "of milliseconds greater than or equal to zero");
    +      }
     
    -    zkStateReader.aliasesHolder.applyModificationAndExportToZk(aliases -> aliases.cloneWithCollectionAlias(aliasName, canonicalCollectionsString));
    +      // Validate we got everything we need
    +      if (routedField == null || routingType == null || start == null || increment == null) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "If any of " + CREATE_ROUTED_ALIAS_PARAMS +
    +            " are supplied, then all of " + REQUIRED_ROUTING_PARAMS + " must be present.");
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +      }
    +
    +      if (!"time".equals(routingType)) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "Only time based routing is supported at this time");
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +      }
    +      // Check for invalid timezone
    +      if(tz != null && !TimeZoneUtils.KNOWN_TIMEZONE_IDS.contains(tz)) {
    +        SolrException solrException = new SolrException(BAD_REQUEST, "Invalid timezone:" + tz);
    +        log.error("Could not create routed alias",solrException);
    +        throw solrException;
    +
    +      }
    +      TimeZone zone;
    +      if (tz != null) {
    +        zone = TimeZoneUtils.getTimeZone(tz);
    +      } else {
    +        zone = TimeZoneUtils.getTimeZone("UTC");
    +      }
    +      DateTimeFormatter fmt = DATE_TIME_FORMATTER.withZone(zone.toZoneId());
    --- End diff --
   
    So if someone specifies start as an iso instant, we don't want to adjust that to match the specified zone? This would lead to start=2018-01-14T20:00:00:00.0000Z&TZ=EST&router.interval=+1DAY it would seem that the first bucket should be 2018-01-15 but this way we would be creating a first bucket for 2018-01-14 instead. That seems confusing. Solr fairly consistently takes the position that times you speak to it must be in UTC, changing that here seems awkward. We could allow more general time format and take the timezone from the entered time, so that they can enter 2018-01-15T00:00:00:00.0000EST which is unlike the rest of how solr handles time AFAIK but would make the correspondence between the collection name and the time value entered intuitive (and maybe complicates combining this with the datemath)
   
    I had originally proposed that collections names for this be UTC and making them full time stamps to avoid all this TZ stuff, but the response (https://issues.apache.org/jira/browse/SOLR-11299?focusedCommentId=16204466&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16204466) was we had to make the collection names friendly for the same reason TZ is provided elsewhere, which means accounting for timezones. (this also leads to yearly double sized hourly partitions and strange mixing at sub-hour partitions, thought that resolution is probably an unusual case)



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12