Deprecate types in rollover index API #38039

mayya-sharipova · 2019-01-30T15:53:26Z

Relates to #35190

elasticmachine · 2019-01-30T15:53:37Z

Pinging @elastic/es-search

jtibshirani

Thanks so much @mayya-sharipova for tackling this API that we missed originally.

Because we missed it in the original PR to add include_type_name to the relevant REST APIs, in this PR I think we also need to update RestRolloverIndexAction to accept a include_type_name parameter, and emit a deprecation warning if it's set. The logic will probably end up looking similar to that in RestCreateIndexAction. I'm guessing the tests are all passing because IndicesClientIT#testRollover happens to never specify mappings, so we don't hit the case where RestRolloverIndexAction has to parse a typeless mapping definition..

I'll also add @hub-cap as a reviewer so that he can take a look at the Java HLRC changes.

jtibshirani · 2019-01-30T23:40:14Z

docs/java-rest/high-level/indices/rollover.asciidoc

@@ -54,9 +54,9 @@ include-tagged::{doc-tests-file}[{api}-request-masterTimeout]
 include-tagged::{doc-tests-file}[{api}-request-waitForActiveShards]
 --------------------------------------------------
 <1> The number of active shard copies to wait for before the rollover index API
-returns a response, as an `int`
+returns a response
 <2> The number of active shard copies to wait for before the rollover index API


Small comment: instead of having the exact same text for both callouts, maybe we could change the second to say "Resets the number of active shard copies to wait for to the default value"?

jtibshirani · 2019-01-30T23:49:16Z

...igh-level/src/test/java/org/elasticsearch/client/indices/rollover/RolloverResponseTests.java

+    }
+
+    private static void toXContent(RolloverResponse response, XContentBuilder builder) throws IOException {
+        builder.startObject();


Instead of duplicating toXContent logic, in recent PRs we have been converting the client-side response to a server response, and then calling toXContent on the server response. Here's an example: https://github.com/elastic/elasticsearch/blob/master/client/rest-high-level/src/test/java/org/elasticsearch/client/indices/GetMappingsResponseTests.java#L102

This is something we only agreed on recently, I didn't catch it on our earlier PRs (like for get field mappings).

yay constant change!!!! :P

jtibshirani · 2019-01-30T23:51:35Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java

@@ -76,6 +74,8 @@
 import org.elasticsearch.client.indices.PutIndexTemplateRequest;
 import org.elasticsearch.client.indices.PutMappingRequest;
 import org.elasticsearch.client.indices.UnfreezeIndexRequest;
+import org.elasticsearch.client.indices.rollover.RolloverRequest;


It would be good to update testRollover to also specify mappings, so we can test our changes related to types.

jtibshirani · 2019-01-30T23:52:34Z

server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponseTests.java

@@ -58,7 +59,7 @@ protected RolloverResponse createTestInstance() {
    private static final List<Supplier<Condition<?>>> conditionSuppliers = new ArrayList<>();
    static {
        conditionSuppliers.add(() -> new MaxAgeCondition(new TimeValue(randomNonNegativeLong())));
-        conditionSuppliers.add(() -> new MaxDocsCondition(randomNonNegativeLong()));
+        conditionSuppliers.add(() -> new MaxSizeCondition(new ByteSizeValue(randomNonNegativeLong())));


jtibshirani · 2019-01-30T23:58:02Z

...rest-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverRequest.java

+ */
+public class RolloverRequest extends TimedRequest implements Validatable, ToXContentObject {
+
+    static final String AGE_CONDITION = "max_age";


I think it might be cleaner if we continued to use the Condition class, as we do in the server-side request class. This would also make the toXContent method more robust/ type safe.

We decided earlier that although we would like to duplicate the request + response classes into client-side versions, we were okay if these classes referenced server objects internally. So we could just use the server-side Condition class here, and not worry about duplicating it as well.

jtibshirani · 2019-01-31T00:00:01Z

client/rest-high-level/src/main/java/org/elasticsearch/client/TimedRequest.java

+     * Sets the timeout to wait for the all the nodes to acknowledge
+     * @param timeout timeout as a string (e.g. 1s)
+     */
+    public void setTimeout(String timeout) {


I touched a few requests that also extended TimedRequest, and opted to just omit these methods (and update the HLRC documentation to remove calls to them). We have been trying to keep these new client-side classes as small as possible, as it is easier to add methods after getting user feedback than to remove them.

jtibshirani · 2019-01-31T00:02:43Z

...rest-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverRequest.java

+/**
+ * Request class to swap index under an alias upon satisfying conditions
+ */
+public class RolloverRequest extends TimedRequest implements Validatable, ToXContentObject {


It would be great to have a test RolloverRequestTests for this new client class. Maybe since the toXContent method is fairly simple, we could just write a few manual tests for serialization, instead of trying to use an xContentTester?

jtibshirani · 2019-01-31T00:04:21Z

...rest-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverRequest.java

+    private CreateIndexRequest createIndexRequest = new CreateIndexRequest("_na_");
+
+    public RolloverRequest(String alias, String newIndexName) {
+        this.alias = alias;


Maybe we can validate that alias is not null here, as we do in the server-side RolloverRequest.

++ to validating all things in a constructor, and making them final as well.

jtibshirani · 2019-01-31T00:12:10Z

...st-high-level/src/test/java/org/elasticsearch/client/indices/RandomCreateIndexGenerator.java

+    public static void randomAliases(CreateIndexRequest request) {
+        int aliasesNo = randomIntBetween(0, 2);
+        for (int i = 0; i < aliasesNo; i++) {
+            request.alias(randomAlias());


Instead of copying over the logic for randomAlias, could we make org.elasticsearch.index.RandomCreateIndexGenerator.randomAlias public and use it?

hub-cap

all minor nits, this is pretty much lgtm for me, but ill let @jtibshirani be the final review.

hub-cap · 2019-02-01T19:02:56Z

...rest-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverRequest.java

+    private CreateIndexRequest createIndexRequest = new CreateIndexRequest("_na_");
+
+    public RolloverRequest(String alias, String newIndexName) {
+        this.alias = alias;


++ to validating all things in a constructor, and making them final as well.

hub-cap · 2019-02-01T19:04:43Z

...est-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverResponse.java

+            (Boolean)args[3], (Boolean)args[4], (Boolean) args[5], (Boolean) args[6]));
+
+    static {
+        PARSER.declareField(constructorArg(), (parser, context) -> parser.text(), OLD_INDEX, ObjectParser.ValueType.STRING);


these can probably be .declareString(constructorArg(), OLD_INDEX), same with the declareBoolean as well.

hub-cap · 2019-02-01T19:05:46Z

...est-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverResponse.java

+    public RolloverResponse(String oldIndex, String newIndex, Map<String, Boolean> conditionResults,
+                            boolean dryRun, boolean rolledOver, boolean acknowledged, boolean shardsAcknowledged) {
+        super(acknowledged, shardsAcknowledged);
+        this.oldIndex = oldIndex;


can you make the fields in the RolloverResponse final pls

hub-cap · 2019-02-01T19:06:42Z

...est-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverResponse.java

+/**
+ * Response object for {@link RolloverRequest} API
+ */
+public final class RolloverResponse extends ShardsAcknowledgedResponse {


does this need to be a ShardsAcknowledgedResponse?

WRT declareAcknowledgedAndShardsAcknowledgedFields, i think we can do something client side thats small and similar? Do we have (m)any classes in o.e.client.indicies using it? Maybe we can put a parent class in o.e.client.indicies that is similar in how it functions.

hub-cap · 2019-02-01T19:07:17Z

...igh-level/src/test/java/org/elasticsearch/client/indices/rollover/RolloverResponseTests.java

+    }
+
+    private static void toXContent(RolloverResponse response, XContentBuilder builder) throws IOException {
+        builder.startObject();


yay constant change!!!! :P

hub-cap · 2019-02-01T19:08:24Z

...est-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverResponse.java

+    }
+
+    @Override
+    protected void addCustomFields(XContentBuilder builder, Params params) throws IOException {


is this needed?

hub-cap · 2019-02-01T19:16:59Z

server/src/main/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponse.java

+/**
+ * Response object for {@link RolloverRequest} API
+ *
+ * Note: there is a new class with the same name for the Java HLRC that uses a typeless format.


hub-cap · 2019-02-01T19:18:24Z

...rest-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverRequest.java

+/**
+ * Request class to swap index under an alias upon satisfying conditions
+ */
+public class RolloverRequest extends TimedRequest implements Validatable, ToXContentObject {


is there nothing to Validate? its fine, just making sure.

…ollover

mayya-sharipova · 2019-02-01T22:17:50Z

@jtibshirani Julie, thanks a lot for such a comprehensive feedback. I have tried to address all your comments except not copying over the logic for randomAlias from org.elasticsearch.index.RandomCreateIndexGenerator.randomAlias. It was not possible as the server's randomAlias expects the server's CreateIndexRequest, that is why we need a copy of that in the client that expects the client's CreateIndexRequest.

@jtibshirani Julie, I particularly wanted to ask your feedback on the way I implemented mapping parsing in the server's RolloverRequest: https://github.com/elastic/elasticsearch/pull/38039/files#diff-2902594b9bc003bcb80ffcb6db13d7a7R74. and in RestRolloverIndexAction https://github.com/elastic/elasticsearch/pull/38039/files#diff-2902594b9bc003bcb80ffcb6db13d7a7R74? Are we happy with this approach? Or would you suggest something else?

@hub-cap Thanks a lot for the review. I will address your feedback in the next commit. I wanted to ask you, why are making the client HLRC Request/Response classes to be again dependent on the server's ones? I remember that before we have made a decision to separate the client HLRC classes from the server code as much as possible, as we reverting this decision now?

jtibshirani

I have tried to address all your comments except not copying over the logic for randomAlias from org.elasticsearch.index.RandomCreateIndexGenerator.randomAlias.

I was just referring to the method randomAlias, which creates an Alias (it doesn't mention CreateIndexRequest at all). It's currently private, but we could make it a public method and call it instead of duplicating that randomization logic.

I particularly wanted to ask your feedback on the way I implemented mapping parsing in the server's RolloverRequest.

It looks good to me overall, I added a suggestion in the code.

I wanted to ask you, why are making the client HLRC Request/Response classes to be again dependent on the server's ones?

I filled @mayya-sharipova in offline about our conversation. I think the short answer is that while the long-term goal is still to cut the server dependency completely, we decided on a compromise for this time-sensitive work where we would create new client requests + responses, but still use some server objects internally.

One other thought I forgot to mention in my first review -- it would be great to add one yml test case, as none of the existing ones for rollover involve mappings.

jtibshirani · 2019-02-01T23:48:12Z

...rest-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverRequest.java

+
+    @Override
+    public Optional<ValidationException> validate() {
+        if (alias == null) {


I think for the new HLRC classes, we've been trying to just validate in the constructor, as opposed to using this special validate method.

you can use Objects.requireNotNull() here.

jtibshirani · 2019-02-02T00:16:46Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestRolloverIndexAction.java

        RolloverRequest rolloverIndexRequest = new RolloverRequest(request.param("index"), request.param("new_index"));
+        if (includeTypeName == false) {
+            rolloverIndexRequest.setNoTypeNameForMappingParsing();


This overall approach looks good to me, but I wonder if instead of setting a special flag on the request here, we can just pass a boolean includeTypeName to the fromXContent method? In particular, parse methods take an arbitary Context parameter that I think you could use to pass in the boolean. Here is an example from FieldCapabilities where we are passing in a String name to the ObjectParser: https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilities.java#L136-L138

mayya-sharipova · 2019-02-04T13:03:54Z

@hub-cap Thanks for the review. The PR is ready for another round of review. I have address all your comments, except that if RolloverResponse class needs to extend ShardsAcknowledgedResponse. It doesn't need, and indeed we can create a client side class that is smaller. But because of the time constraints for the feature freeze, would be ok to leave this work for later?

@jtibshirani Thanks for another round of review, I have tried to address your comments in the last commit. Can you please review again

hub-cap

LGTM, i didnt see an answer on the class extending ShardsAcknowledgedResponse but i dont think thats a deal breaker. one other nit inline

hub-cap · 2019-02-04T15:35:56Z

...rest-high-level/src/main/java/org/elasticsearch/client/indices/rollover/RolloverRequest.java

+
+    @Override
+    public Optional<ValidationException> validate() {
+        if (alias == null) {


you can use Objects.requireNotNull() here.

jtibshirani

This looks good to me! I'll also take a look at the question around ShardsAcknowledgedResponse (for a future PR), as I ended up using it for CreateIndexResponse as well.

jtibshirani · 2019-02-04T18:16:55Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.rollover/40_mapping.yml

+  - do:
+      index:
+        index: logs-1
+        type:  test


I'm not sure it's worth another update + build, but a note that we've been trying to avoid typed index calls (unless we're explicitly testing typed behavior as in 41_mapping_with_types).

mayya-sharipova · 2019-02-04T19:20:01Z

@jtibshirani Thanks Julie for the review.,
I have decided to trigger another update+ build to address your comment for the types in yml tests.

About ShardsAcknowledgedResponse, and I talked offline with @hub-cap, and we have decided it would be ok to address this in a separate PR. I have filed an issue for this

Backport for elastic#38039 Relates to elastic#35190

…-lease-expiration * elastic/master: (24 commits) Add support for API keys to access Elasticsearch (elastic#38291) Add typless client side GetIndexRequest calls and response class (elastic#37778) Limit token expiry to 1 hour maximum (elastic#38244) add docs saying mixed-cluster ILM is not supported (elastic#37954) Skip unsupported languages for tests (elastic#38328) Deprecate `_type` in simulate pipeline requests (elastic#37949) Mute testCannotShrinkLeaderIndex (elastic#38374) Tighten mapping syncing in ccr remote restore (elastic#38071) Add test for `PutFollowAction` on a closed index (elastic#38236) Fix SSLContext pinning to TLSV1.2 in reload tests (elastic#38341) Mute RareClusterStateIT.testDelayedMappingPropagationOnReplica (elastic#38357) Deprecate types in rollover index API (elastic#38039) Types removal - fix FullClusterRestartIT warning expectations (elastic#38310) Fix ILM explain response to allow unknown fields (elastic#38054) Mute testFollowIndexAndCloseNode (elastic#38360) Docs: Drop inline callout from scroll example (elastic#38340) Deprecate HLRC security methods (elastic#37883) Remove types from Monitoring plugin "backend" code (elastic#37745) Add Composite to AggregationBuilders (elastic#38207) Clarify slow cluster-state log messages (elastic#38302) ...

Backport for #38039 Relates to #35190

Backport for elastic#38039 Relates to elastic#35190

Backport for #38039 Relates to #35190

Deprecate types in rollover index API

4963aba

Relates to elastic#35190

mayya-sharipova added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Jan 30, 2019

mayya-sharipova added >deprecation v7.0.0 labels Jan 30, 2019

mayya-sharipova requested a review from jtibshirani January 30, 2019 17:30

jtibshirani reviewed Jan 31, 2019

View reviewed changes

jtibshirani mentioned this pull request Jan 31, 2019

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

jtibshirani requested a review from hub-cap January 31, 2019 01:39

hub-cap reviewed Feb 1, 2019

View reviewed changes

mayya-sharipova added 2 commits February 1, 2019 15:32

Address Julie's comments

1861ba7

Merge remote-tracking branch 'upstream/master' into deprecate-types-r…

cd68a95

…ollover

jtibshirani reviewed Feb 2, 2019

View reviewed changes

mayya-sharipova added 2 commits February 4, 2019 07:57

Address Michael's and Julie's comments

efc47df

Merge branch 'master' into deprecate-types-rollover

2bcf033

hub-cap approved these changes Feb 4, 2019

View reviewed changes

jtibshirani approved these changes Feb 4, 2019

View reviewed changes

Address Julie's comments 2

5a4888c

mayya-sharipova mentioned this pull request Feb 4, 2019

Make a Java HLRC client version of ShardsAcknowledgedResponse #38355

Closed

mayya-sharipova merged commit 6417044 into elastic:master Feb 4, 2019

mayya-sharipova deleted the deprecate-types-rollover branch February 4, 2019 21:07

mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this pull request Feb 5, 2019

Deprecate types in rollover index API

d1f2cb1

Backport for elastic#38039 Relates to elastic#35190

mayya-sharipova mentioned this pull request Feb 5, 2019

Deprecate types in rollover index API #38389

Merged

mayya-sharipova added a commit that referenced this pull request Feb 5, 2019

Deprecate types in rollover index API (#38389)

2e7b7a8

Backport for #38039 Relates to #35190

mayya-sharipova mentioned this pull request Feb 5, 2019

Deprecate types in rollover index API (#38389) #38458

Merged

mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this pull request Feb 5, 2019

Deprecate types in rollover index API (elastic#38389)

34e81e3

Backport for elastic#38039 Relates to elastic#35190

mayya-sharipova added a commit that referenced this pull request Feb 5, 2019

Deprecate types in rollover index API (#38389) (#38458)

aa9816c

Backport for #38039 Relates to #35190

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate types in rollover index API #38039

Deprecate types in rollover index API #38039

mayya-sharipova commented Jan 30, 2019

elasticmachine commented Jan 30, 2019

jtibshirani left a comment •

edited

Loading

jtibshirani Jan 30, 2019

jtibshirani Jan 30, 2019

hub-cap Feb 1, 2019

jtibshirani Jan 30, 2019 •

edited

Loading

jtibshirani Jan 30, 2019

jtibshirani Jan 30, 2019

jtibshirani Jan 31, 2019

jtibshirani Jan 31, 2019

jtibshirani Jan 31, 2019

hub-cap Feb 1, 2019

jtibshirani Jan 31, 2019

hub-cap left a comment

hub-cap Feb 1, 2019

hub-cap Feb 1, 2019

hub-cap Feb 1, 2019

hub-cap Feb 1, 2019

hub-cap Feb 1, 2019

hub-cap Feb 1, 2019

hub-cap Feb 1, 2019

hub-cap Feb 1, 2019

hub-cap Feb 1, 2019

mayya-sharipova commented Feb 1, 2019

jtibshirani left a comment •

edited

Loading

jtibshirani Feb 1, 2019

hub-cap Feb 4, 2019

jtibshirani Feb 2, 2019

mayya-sharipova commented Feb 4, 2019 •

edited

Loading

hub-cap left a comment

hub-cap Feb 4, 2019

jtibshirani left a comment •

edited

Loading

jtibshirani Feb 4, 2019

mayya-sharipova commented Feb 4, 2019

Deprecate types in rollover index API #38039

Deprecate types in rollover index API #38039

Conversation

mayya-sharipova commented Jan 30, 2019

elasticmachine commented Jan 30, 2019

jtibshirani left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented Feb 1, 2019

jtibshirani left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented Feb 4, 2019 • edited Loading

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented Feb 4, 2019

jtibshirani left a comment •

edited

Loading

jtibshirani Jan 30, 2019 •

edited

Loading

jtibshirani left a comment •

edited

Loading

mayya-sharipova commented Feb 4, 2019 •

edited

Loading

jtibshirani left a comment •

edited

Loading