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.

Sign up for GitHub

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

Jump to bottom

Normalized prefix for rollover API #57271

Merged
merged 7 commits into from
Jun 16, 2020
Merged

Normalized prefix for rollover API #57271

merged 7 commits into from
Jun 16, 2020

Conversation

Gaurav614
Copy link
Contributor

It fixes the issue #53388
by normalizing prefix at index creation request itself

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

@Gaurav614
Normalized prefix for rollover API
2e33171 
It fixes the issue elastic#53388
by normalizing prefix at index creation request itself
@dliappis dliappis added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label May 28, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/ILM+SLM)

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label May 28, 2020
@dakrone dakrone self-requested a review May 28, 2020 14:57
dakrone
dakrone requested changes Jun 1, 2020
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @Gaurav614! I left a couple of comments, if you could address those then I will merge this in

...src/internalClusterTest/java/org/elasticsearch/action/admin/indices/rollover/RolloverIT.java Outdated
@@ -154,7 +154,7 @@ public void testRolloverWithIndexSettings() throws Exception {
indexDoc("test_index-2", "1", "field", "value");
flush("test_index-2");
final Settings settings = Settings.builder()
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
.put("number_of_shards", 1) // testing without "index" prefix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer a dedicated test for this, just so that it doesn't get accidentally removed and the behavior reverted in the future, could you add a test for this? (it can basically be a copy of this test with a descriptive name)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, made a separate test case for it !

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java Outdated
@@ -306,12 +303,20 @@ public void onFailure(String source, Exception e) {
});
}

private void applyCreateIndexRequestInternal(CreateIndexClusterStateUpdateRequest createIndexClusterStateRequest){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this "apply" I think is a little misleading, I think normalizeRequestSettings would be a better name for this method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

@dakrone
Copy link
Member

dakrone commented Jun 1, 2020

@elasticmachine ok to test

@Gaurav614 Gaurav614 requested a review from dakrone June 10, 2020 13:50
dakrone
dakrone approved these changes Jun 10, 2020
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the changes @Gaurav614, I'll take care of merging and backporting this.

@dakrone
Copy link
Member

dakrone commented Jun 10, 2020

@elasticmachine ok to test

@dakrone
Copy link
Member

dakrone commented Jun 10, 2020

Looks like checkstyle is unhappy about this line:

[ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request-1/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java:311: Line is longer than 140 characters (found 155). [LineLength]

@Gaurav614 can you shorten the line to < 140 characters?

@Gaurav614
Copy link
Contributor Author

Gaurav614 commented Jun 12, 2020 •
edited
Loading

Looks like checkstyle is unhappy about this line:

[ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request-1/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java:311: Line is longer than 140 characters (found 155). [LineLength]

@Gaurav614 can you shorten the line to < 140 characters?

Oops!!

I ran gradle target for check style check and it was successful.

elasticsearch $ ./gradlew spotlessJavaCheck
Starting a Gradle Daemon (subsequent builds will be faster)
=======================================
Elasticsearch Build Hamster says Hello!
  Gradle Version        : 6.5
  OS Info               : Mac OS X 10.14.6 (x86_64)
  Runtime JDK Version   : 11 (Oracle JDK)
  Runtime java.home     : /Library/Java/JavaVirtualMachines/jdk-11.0.5.jdk/Contents/Home
  Gradle JDK Version    : 14 (OpenJDK)
  Gradle java.home      : /Library/Java/JavaVirtualMachines/jdk-14.jdk/Contents/Home
  Random Testing Seed   : 86ACC3E61ADF00D9
  In FIPS 140 mode      : false
=======================================

BUILD SUCCESSFUL in 1m 9s
45 actionable tasks: 45 executed

@dakrone
Copy link
Member

dakrone commented Jun 12, 2020

@elasticmachine ok to test

@dakrone
Copy link
Member

dakrone commented Jun 12, 2020

@elasticmachine update branch

@dakrone dakrone merged commit 69e1c06 into elastic:master Jun 16, 2020
@dakrone dakrone mentioned this pull request Jun 16, 2020
Merged
dakrone pushed a commit to dakrone/elasticsearch that referenced this pull request Jun 16, 2020
@Gaurav614 @dakrone
Normalized prefix for rollover API (elastic#57271)
c445a17 
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Lee Hinman <[email protected]>

It fixes the issue elastic#53388
by normalizing prefix at index creation request itself
dakrone added a commit that referenced this pull request Jun 16, 2020
@dakrone @Gaurav614
[7.x] Normalized prefix for rollover API (#57271) (69e1c06) (#58171)
03ce0f8 
* Normalized prefix for rollover API (#57271)

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Lee Hinman <[email protected]>

It fixes the issue #53388
by normalizing prefix at index creation request itself

* Fix compilation for backport

Co-authored-by: Gaurav Chandani <[email protected]>
@pugnascotia pugnascotia mentioned this pull request Jul 16, 2020
Closed
@occidere occidere mentioned this pull request Jan 25, 2021
Open
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dakrone dakrone dakrone approved these changes

Assignees
No one assigned
Labels
>bug :Data Management/ILM+SLM Index and Snapshot lifecycle management Team:Data Management Meta label for data/management team v7.9.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Gaurav614 @elasticmachine @dakrone @jakelandis @dliappis @pugnascotia