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

Improved IndexNotFoundException's default error message #34649

Merged
merged 6 commits into from
Oct 24, 2018
Merged

Improved IndexNotFoundException's default error message #34649

merged 6 commits into from
Oct 24, 2018

Conversation

atapin
Copy link
Contributor

@atapin atapin commented Oct 19, 2018

The message is improved by including the missing index name. Fixes #34628.

@s1monw s1monw added the :Core/Infra/Core Core issues without another label label Oct 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

s1monw
s1monw reviewed Oct 19, 2018
View reviewed changes
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

left one comment

server/src/main/java/org/elasticsearch/index/IndexNotFoundException.java Outdated
@@ -37,7 +40,7 @@ public IndexNotFoundException(String index) {
}

public IndexNotFoundException(String index, Throwable cause) {
super("no such index", cause);
super(makeMessage(index), cause);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be ok to duplicate the message like this:

Suggested change
super(makeMessage(index), cause);
super("no such index ["+ index + "]", cause);

Copy link
Contributor

Choose a reason for hiding this comment

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

we also have a convention to embed things like index names in [ ]

@s1monw
Copy link
Contributor

s1monw commented Oct 19, 2018

@elasticmachine ok to test

@atapin atapin force-pushed the 34628-improve-indexnotfoundexception-message branch from 190e09d to 332fdd8 Compare October 22, 2018 22:56
@atapin
Copy link
Contributor Author

atapin commented Oct 23, 2018 •
edited
Loading

Blocked by #34731

vladimirdolzhenko
vladimirdolzhenko reviewed Oct 23, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/index/IndexNotFoundException.java Outdated
@@ -46,7 +46,7 @@ public IndexNotFoundException(Index index) {
}

public IndexNotFoundException(Index index, Throwable cause) {
super("no such index", cause);
super("no such index " + index.toString(), cause);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "no such index [" + index.getName() + "]" as index.toString() could return smth like [index42/n6gzFZTgS664GUfx0Xrpjw] that is a bit inconsistent with other messages

@vladimirdolzhenko vladimirdolzhenko mentioned this pull request Oct 23, 2018
Closed
@atapin atapin force-pushed the 34628-improve-indexnotfoundexception-message branch from 332fdd8 to 58ab72c Compare October 23, 2018 13:28
@atapin atapin force-pushed the 34628-improve-indexnotfoundexception-message branch from 58ab72c to 3ab8f53 Compare October 24, 2018 07:45
@vladimirdolzhenko
Copy link
Contributor

vladimirdolzhenko commented Oct 24, 2018 •
edited
Loading

@atapin great job. there are as well yaml-based tests, failed test points to 25_no_auto_create.yml that still expects exception messages w/o index specific.

@atapin
Copy link
Contributor Author

atapin commented Oct 24, 2018

@vladimirdolzhenko oops I missed that guy. thanks for the hint.

@rjernst rjernst merged commit 5f58818 into elastic:master Oct 24, 2018
@rjernst
Copy link
Member

rjernst commented Oct 24, 2018

Thanks for the contribution @atapin!

rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Oct 24, 2018
@atapin @rjernst
Improve IndexNotFoundException's default error message (elastic#34649)
95d2348 
This commit adds the index name to the error message when an index is not found.
@rjernst rjernst mentioned this pull request Oct 24, 2018
Closed
@atapin atapin deleted the 34628-improve-indexnotfoundexception-message branch October 25, 2018 07:07
kcm pushed a commit that referenced this pull request Oct 30, 2018
@atapin @kcm
Improve IndexNotFoundException's default error message (#34649)
0c50308 
This commit adds the index name to the error message when an index is not found.
@Mpdreamz Mpdreamz mentioned this pull request Dec 13, 2018
Closed
@rjernst rjernst removed the v6.5.0 label Jan 15, 2019
@rjernst
Copy link
Member

rjernst commented Jan 15, 2019

Note that this was not actually included in 6.5.0 (I have now removed the label). The backport had failures which were difficult to track down, and at this point I do not see a compelling reason to investigate given prereleases of 7.0 are being built.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s1monw s1monw s1monw left review comments

@rjernst rjernst rjernst left review comments

@vladimirdolzhenko vladimirdolzhenko vladimirdolzhenko left review comments

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label >enhancement v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@atapin @elasticmachine @s1monw @vladimirdolzhenko @rjernst @colings86