Skip to content

Normalized prefix for rollover API #57271

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
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
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
});
}

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 <elasticmachine@users.noreply.github.com> Co-authored-by: Lee Hinman <lee@writequit.org> 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 <elasticmachine@users.noreply.github.com> Co-authored-by: Lee Hinman <lee@writequit.org> It fixes the issue #53388 by normalizing prefix at index creation request itself * Fix compilation for backport Co-authored-by: Gaurav Chandani <chngau@amazon.com>
@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

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

6 participants

@Gaurav614 @elasticmachine @dakrone @jakelandis @dliappis @pugnascotia