Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create index request should return the index name #25139

Merged
merged 8 commits into from Jun 9, 2017

Conversation

Projects
None yet
4 participants
@s12v
Copy link
Contributor

commented Jun 8, 2017

Implements #23044

Based on #23061 which is closed

TODO

  • Need to define version for this feature (for serialization)
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jun 8, 2017

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@@ -32,28 +33,37 @@
public class CreateIndexResponse extends AcknowledgedResponse {

private boolean shardsAcked;
private String index;
private Version minimumVersionWithIndex = Version.CURRENT; // TODO set version

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 8, 2017

Member

You can set it to 5.6.0.

This comment has been minimized.

Copy link
@s12v

s12v Jun 8, 2017

Author Contributor

@jasontedor, done

@@ -32,28 +33,37 @@
public class CreateIndexResponse extends AcknowledgedResponse {

private boolean shardsAcked;
private String index;
private Version minimumVersionWithIndex = Version.V_5_6_0;

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 8, 2017

Member

Also, no need for the field, you can inline it in the places it is used.

This comment has been minimized.

Copy link
@s12v

s12v Jun 8, 2017

Author Contributor

@jasontedor, hmmm, that way they will not diverge. Also variable name explains what's going on. But as you say :)

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 8, 2017

Member

They shouldn't diverge as these will never change other than eventually being removed when we no longer have to be backwards compatible with 5.6.0 (say, 7.0.0). This pattern occurs often enough that the field is not needed to clarify that this was added in 5.6.0 instead the code already speaks for itself in that regard. Finally, the git blame will tell us the commit that added this which will have linkage back to this PR if someone really needs to go on a cave expedition through that. 😄

This comment has been minimized.

Copy link
@s12v

s12v Jun 8, 2017

Author Contributor

@jasontedor, I see. Thanks for the explanation :)

@@ -31,6 +31,16 @@
- match: { test_index.settings.index.number_of_replicas: "0"}

---
"Create index":

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 8, 2017

Member

This will need a skip because it will fail the backwards compatibility tests without it. Typically what we would do is skip through to 5.99.99, then we backport this and set the skip to 5.5.99, then we change the skip in master to 5.5.99. (We handle the backporting, so you can simply set the skip to 5.99.99 in this PR.)

This comment has been minimized.

Copy link
@s12v

s12v Jun 8, 2017

Author Contributor

@jasontedor, added skip. Could you please check?

listener.onResponse(new ShrinkResponse(response.isAcknowledged(), response.isShardsAcked())), listener::onFailure));
listener.onResponse(
new ShrinkResponse(response.isAcknowledged(), response.isShardsAcked(), updateRequest.index())
),

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 8, 2017

Member

The line breaks are awkward here, can we put each parameter to createIndexService#createIndex on a single line or wrap them a little differently?

This comment has been minimized.

Copy link
@s12v

s12v Jun 8, 2017

Author Contributor

@jasontedor, updated

@jasontedor

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

This is good, I'll start a CI run.

@jasontedor

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

test this please

"Create index":
- skip:
version: " - 5.99.99"
reason: create index response contains index name since 5.6.0

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 8, 2017

Member

Yes, this is good.

ActionListener.wrap(response ->
listener.onResponse(
new ShrinkResponse(response.isAcknowledged(), response.isShardsAcked(), updateRequest.index())
),

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 8, 2017

Member

Sorry to nit on this, I hate formatting nits too but we still have to keep the codebase clean, but I think it should fit like this:

diff --git a/core/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportShrinkAction.java b/core/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportShrinkAction.java
index 9c6855c1a5..2555299709 100644
--- a/core/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportShrinkAction.java
+++ b/core/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportShrinkAction.java
@@ -94,9 +94,7 @@ public class TransportShrinkAction extends TransportMasterNodeAction<ShrinkReque
                 createIndexService.createIndex(
                     updateRequest,
                     ActionListener.wrap(response ->
-                        listener.onResponse(
-                            new ShrinkResponse(response.isAcknowledged(), response.isShardsAcked(), updateRequest.index())
-                        ),
+                        listener.onResponse(new ShrinkResponse(response.isAcknowledged(), response.isShardsAcked(), updateRequest.index())),
                         listener::onFailure
                     )
                 );

This comment has been minimized.

Copy link
@s12v

s12v Jun 8, 2017

Author Contributor

@jasontedor, no worries :) updated

@jasontedor

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

retest this please

@jasontedor

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

retest this please

@s12v

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2017

@jasontedor, some tests are still failing. For example, this one:

gradle :qa:mixed-cluster:v5.6.0-SNAPSHOT#mixedClusterTestRunner -Dtests.seed=408700B2D538D4D -Dtests.class=org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT -Dtests.method="test {p0=indices.stats/15_types/Types - star}" -Dtests.security.manager=true -Dtests.locale=ca-ES -Dtests.timezone=America/Atikokan

with this error:

"type" : "transport_serialization_exception",
  1>         "reason" : "Failed to deserialize response of type [org.elasticsearch.action.admin.indices.create.CreateIndexResponse]",
  1>         "caused_by" : {
  1>           "type" : "index_out_of_bounds_exception",
  1>           "reason" : "readerIndex(23) + length(1) exceeds writerIndex(23): UnpooledDuplicatedByteBuf(ridx: 23, widx: 23, cap: 65536, unwrapped: PooledHeapByteBuf(ridx: 6, widx: 23, cap: 65536))",

I'm wondering how this test works. Does it download 5.6.0-SNAPSHOT from somewhere else? In this case we assume 5.6.0 supports it, but it's not.

Could you please help me to fix it? Placing in.getVersion().onOrAfter(Version.CURRENT) seems to help, but this is not what we want, right?

@jasontedor

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

Yeah, let's do this. For now set it to 6.0.0-alpha3. We'll backport this to 5.6.0 and after the backport change the version to 5.6.0.

Does it download 5.6.0-SNAPSHOT from somewhere else?

We checkout the 5.x branch and build it on the fly as part of the build.

@jasontedor

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

retest this please

@jasontedor
Copy link
Member

left a comment

LGTM!

@jasontedor

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

Thanks @s12v.

@jasontedor jasontedor merged commit 7c8657d into elastic:master Jun 9, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author has signed the CLA
Details

jasontedor added a commit that referenced this pull request Jun 9, 2017

Return the index name on a create index response
This commit modifies the create index response so that it includes the
index name.

Relates #25139
@jasontedor

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

I integrated:

@s12v s12v deleted the s12v:index-name-in-create-index-response branch Jun 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.