Generify Index and Shard exceptions #12023
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -89,16 +91,18 @@ public ElasticsearchException(StreamInput in) throws IOException { | |||
| * Adds a new header with the given key. | |||
| * This method will replace existing header if a header with the same key already exists | |||
| */ | |||
| public void addHeader(String key, String... value) { | |||
| public ElasticsearchException addHeader(String key, String... value) { | |||
|
this is a great start! left some comments |
|
I added a new commit @uboness |
| } | ||
|
|
||
| private static final String INDEX_HEADER_KEY = "es.internal.index"; | ||
| private static final String SHARD_HEADER_KEY = "es.internal.shard"; |
There was a problem hiding this comment.
maybe remove the internal here?
|
I went through this and left some comments here and there. My main concern is that we are inconsistent in using IndexNotFoundException and it's base class ResourceNotFoundException (with it's index set). I think we should make a choice and be consistent. I tend to using a dedicated IndexNotFoundException. I think it will make the code easier to read. |
| } | ||
|
|
||
| public void setIndex(Index index) { | ||
| if (index != null) { |
There was a problem hiding this comment.
if index == null, do we want to clear the header?
|
Thx @s1monw . Left some minor comments which I don't think merit another round. LGTM. |
s1monw
force-pushed
the
add_resource_not_found_exception
branch
2 times, most recently
from
68a1791
to
9174db9
Compare
Today we have a intermediate hierarchy for shard and index exceptions which makes it hard to introduce generic exceptions like ResourceNotFoundException intoduced in this commit. This commit breaks up the hierarchy by adding index and shard as a special internal header that gets rendered for every exception that fills that header. This commit removes dedicated exceptions like `IndexMissingException` or `IndexShardMissingException` in favour of `ResourceNotFoundException`
s1monw
force-pushed
the
add_resource_not_found_exception
branch
from
9174db9
to
7db293c
Compare
lcawl
added
:Core/Infra/Core
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today we have a intermediate hierarchy for shard and index exceptions
which makes it hard to introduce generic exceptions like ResourceNotFoundException
intoduced in this commit. This commit breaks up the hierarchy by adding index and shard
as a special internal header that gets rendered for every exception that fills that header.
This commit removes dedicated exceptions like
IndexMissingExceptionorIndexShardMissingExceptionin favour ofResourceNotFoundException