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

Replace IndexAlreadyExistsException with ResourceAlreadyExistsException #21494

Merged
merged 2 commits into from Nov 17, 2016

Conversation

Projects
None yet
6 participants
@dimitris-athanasiou
Contributor

dimitris-athanasiou commented Nov 11, 2016

Similar to ResourceNotFoundException, a generic ResourceAlreadyExistsException will reduce the need to introduce an additional exception for each type of resource that can have that error.

At the moment, there is:

  • IndexAlreadyExistsException
  • IndexTemplateAlreadyExistsException
  • IndexShardAlreadyExistsException

This commit changes those to inherit ResourceAlreadyExistsException
as removing them would cause backwards compatibility problems.

@dimitris-athanasiou

This comment has been minimized.

Show comment
Hide comment
@dimitris-athanasiou

dimitris-athanasiou Nov 11, 2016

Contributor

Note that in 6.0 we could decide to delete the Index*AlreadyExistsException classes. We'd still need to keep them in 5.x for backwards compatibility. Let me know what you think and I'll adjust the PR respectively.

Contributor

dimitris-athanasiou commented Nov 11, 2016

Note that in 6.0 we could decide to delete the Index*AlreadyExistsException classes. We'd still need to keep them in 5.x for backwards compatibility. Let me know what you think and I'll adjust the PR respectively.

Show outdated Hide outdated core/src/main/java/org/elasticsearch/ResourceAlreadyExistsException.java
@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Nov 11, 2016

Member

I think that @s1monw's thinking should be taken into consideration before this PR is integrated.

Member

jasontedor commented Nov 11, 2016

I think that @s1monw's thinking should be taken into consideration before this PR is integrated.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Nov 11, 2016

Contributor

I think this PR should remove all the exceptions and replace it by ResourceAlreadyExistsException. We can't add new exceptions in minors since it's basically a wire protocol break. What we can do is to rename an existing exception to ResourceAlreadyExistsException and deprecate all the other ones we have. In 6.0 we can remove them and for 5.x we rename IndexAlreadyExistsException to ResourceAlreadyExistsException and deprecate the usage of IndexShardAlreadyExistsException and IndexTemplateAlreadyExistsException. I really think we can replace most of them with a simple IllegalStateException or IllegalArgumentException? Ideally, we don't add a new one at all :)

Contributor

s1monw commented Nov 11, 2016

I think this PR should remove all the exceptions and replace it by ResourceAlreadyExistsException. We can't add new exceptions in minors since it's basically a wire protocol break. What we can do is to rename an existing exception to ResourceAlreadyExistsException and deprecate all the other ones we have. In 6.0 we can remove them and for 5.x we rename IndexAlreadyExistsException to ResourceAlreadyExistsException and deprecate the usage of IndexShardAlreadyExistsException and IndexTemplateAlreadyExistsException. I really think we can replace most of them with a simple IllegalStateException or IllegalArgumentException? Ideally, we don't add a new one at all :)

@dimitris-athanasiou

This comment has been minimized.

Show comment
Hide comment
@dimitris-athanasiou

dimitris-athanasiou Nov 13, 2016

Contributor

There are a few points here that I would like to understand better:

  • IndexShardAlreadyExistsException has a 500 HTTP status code. The other two have 400. As the one is server error and the other two user error, should we keep them separate? Or can we simply make ResourceAlreadyExistsException have a variable status code?
  • If we rename IndexAlreadyExistsException to ResourceNotFoundException in 5.x aren't we already breaking BWC? Should we consider keeping all the IndexXxxAlreadyExistsException for 5.x as they are and only remove them in 6.0?
  • Whatever decision we make for this one, should we also apply it for XxxNotFoundException? There is IndexNotFoundException, ShardNotFoundException, AliasesNotFoundException and ResourceNotFoundException.
Contributor

dimitris-athanasiou commented Nov 13, 2016

There are a few points here that I would like to understand better:

  • IndexShardAlreadyExistsException has a 500 HTTP status code. The other two have 400. As the one is server error and the other two user error, should we keep them separate? Or can we simply make ResourceAlreadyExistsException have a variable status code?
  • If we rename IndexAlreadyExistsException to ResourceNotFoundException in 5.x aren't we already breaking BWC? Should we consider keeping all the IndexXxxAlreadyExistsException for 5.x as they are and only remove them in 6.0?
  • Whatever decision we make for this one, should we also apply it for XxxNotFoundException? There is IndexNotFoundException, ShardNotFoundException, AliasesNotFoundException and ResourceNotFoundException.

s1monw added a commit to s1monw/elasticsearch that referenced this pull request Nov 14, 2016

Remove `IndexTemplateAlreadyExistsException` and `IndexShardAlreadyEx…
…istsException`

Both exception can be replaced with java built-in exception, IAE and ISE respectively.
This should be backported partially to 5.x which the transport layer code should be preserved.

Relates to #21494
@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Nov 14, 2016

Contributor

Whatever decision we make for this one, should we also apply it for XxxNotFoundException? There is IndexNotFoundException, ShardNotFoundException, AliasesNotFoundException and ResourceNotFoundException.

lets not worry about this for now

IndexShardAlreadyExistsException has a 500 HTTP status code. The other two have 400. As the one is server error and the other two user error, should we keep them separate? Or can we simply make ResourceAlreadyExistsException have a variable status code?

I just looked into the code and replaced IndexShardAlreadyExistsException with IllegalStateException which maps to 500.. yet, I don't think this is important since it won't bubble up anyway. I also removed IndexTemplateAlreadyExistsException which is an IllegalArgumentException IMO. I can open up a PR soon for those two

If we rename IndexAlreadyExistsException to ResourceNotFoundException in 5.x aren't we already breaking BWC? Should we consider keeping all the IndexXxxAlreadyExistsException for 5.x as they are and only remove them in 6.0?

so on the wire protocol it will be just fine since we are not name-aware, we use an integer ID to deserialize. If you are thinking of REST layer we can just do something like this:

/*
 * Licensed to Elasticsearch under one or more contributor
 * license agreements. See the NOTICE file distributed with
 * this work for additional information regarding copyright
 * ownership. Elasticsearch licenses this file to you under
 * the Apache License, Version 2.0 (the "License"); you may
 * not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *    http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */

package org.elasticsearch.indices;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.index.Index;
import org.elasticsearch.rest.RestStatus;

import java.io.IOException;

public class ResourceAlreadyExistsException extends ElasticsearchException {

    public ResourceAlreadyExistsException(Index index, String message) {
        super(message);
        setIndex(index);
    }

    public ResourceAlreadyExistsException(StreamInput in) throws IOException{
        super(in);
    }

    @Override
    public RestStatus status() {
        return RestStatus.BAD_REQUEST;
    }

    @Override
    protected String getExceptionName() {
        if (getIndex() != null) {
            // this is only for BWC and can be removed in 6.0.0
            return "index_already_exists_exception";
        } else {
            return super.getExceptionName();
        }
    }
}
Contributor

s1monw commented Nov 14, 2016

Whatever decision we make for this one, should we also apply it for XxxNotFoundException? There is IndexNotFoundException, ShardNotFoundException, AliasesNotFoundException and ResourceNotFoundException.

lets not worry about this for now

IndexShardAlreadyExistsException has a 500 HTTP status code. The other two have 400. As the one is server error and the other two user error, should we keep them separate? Or can we simply make ResourceAlreadyExistsException have a variable status code?

I just looked into the code and replaced IndexShardAlreadyExistsException with IllegalStateException which maps to 500.. yet, I don't think this is important since it won't bubble up anyway. I also removed IndexTemplateAlreadyExistsException which is an IllegalArgumentException IMO. I can open up a PR soon for those two

If we rename IndexAlreadyExistsException to ResourceNotFoundException in 5.x aren't we already breaking BWC? Should we consider keeping all the IndexXxxAlreadyExistsException for 5.x as they are and only remove them in 6.0?

so on the wire protocol it will be just fine since we are not name-aware, we use an integer ID to deserialize. If you are thinking of REST layer we can just do something like this:

/*
 * Licensed to Elasticsearch under one or more contributor
 * license agreements. See the NOTICE file distributed with
 * this work for additional information regarding copyright
 * ownership. Elasticsearch licenses this file to you under
 * the Apache License, Version 2.0 (the "License"); you may
 * not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *    http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */

package org.elasticsearch.indices;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.index.Index;
import org.elasticsearch.rest.RestStatus;

import java.io.IOException;

public class ResourceAlreadyExistsException extends ElasticsearchException {

    public ResourceAlreadyExistsException(Index index, String message) {
        super(message);
        setIndex(index);
    }

    public ResourceAlreadyExistsException(StreamInput in) throws IOException{
        super(in);
    }

    @Override
    public RestStatus status() {
        return RestStatus.BAD_REQUEST;
    }

    @Override
    protected String getExceptionName() {
        if (getIndex() != null) {
            // this is only for BWC and can be removed in 6.0.0
            return "index_already_exists_exception";
        } else {
            return super.getExceptionName();
        }
    }
}

s1monw added a commit that referenced this pull request Nov 14, 2016

Remove `IndexTemplateAlreadyExistsException` and `IndexShardAlreadyEx…
…istsException` (#21539)

Both exception can be replaced with java built-in exception, IAE and ISE respectively.
This should be back ported partially to 5.x which the transport layer code should be preserved.

Relates to #21494
@dimitris-athanasiou

This comment has been minimized.

Show comment
Hide comment
@dimitris-athanasiou

dimitris-athanasiou Nov 14, 2016

Contributor

Sounds good, I will update the PR.

I summarise the decision as I understand it:

  • 6.0: Rename IndexAlreadyExistsException to ResourceAlreadyExistsException
  • 5.x: Rename IndexAlreadyExistsException to ResourceAlreadyExistsException but maintain the exception name to index_already_exists_exception when an index is set.
Contributor

dimitris-athanasiou commented Nov 14, 2016

Sounds good, I will update the PR.

I summarise the decision as I understand it:

  • 6.0: Rename IndexAlreadyExistsException to ResourceAlreadyExistsException
  • 5.x: Rename IndexAlreadyExistsException to ResourceAlreadyExistsException but maintain the exception name to index_already_exists_exception when an index is set.

s1monw added a commit that referenced this pull request Nov 15, 2016

Remove `IndexTemplateAlreadyExistsException` and `IndexShardAlreadyEx…
…istsException` (#21539)

Both exception can be replaced with java built-in exception, IAE and ISE respectively.
This should be back ported partially to 5.x which the transport layer code should be preserved.

Relates to #21494
@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Nov 15, 2016

Contributor

++ sounds good to me @jasontedor @nik9000 WDYT?

Contributor

s1monw commented Nov 15, 2016

++ sounds good to me @jasontedor @nik9000 WDYT?

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Nov 15, 2016

Contributor

Makes sense to me.

Contributor

nik9000 commented Nov 15, 2016

Makes sense to me.

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Nov 15, 2016

Member

++ sounds good to me @jasontedor @nik9000 WDYT?

Makes sense to me.

+1

Member

jasontedor commented Nov 15, 2016

++ sounds good to me @jasontedor @nik9000 WDYT?

Makes sense to me.

+1

@dimitris-athanasiou dimitris-athanasiou changed the title from Introduce ResourceAlreadyExistsException to Replace IndexAlreadyExistsException with ResourceAlreadyExistsException Nov 16, 2016

@dimitris-athanasiou

This comment has been minimized.

Show comment
Hide comment
@dimitris-athanasiou

dimitris-athanasiou Nov 16, 2016

Contributor

PR is now updated with latest plan.

Contributor

dimitris-athanasiou commented Nov 16, 2016

PR is now updated with latest plan.

@s1monw

left one comment otherwise looks good

@@ -745,7 +745,7 @@ public void testIds() {
ids.put(120, org.elasticsearch.repositories.RepositoryVerificationException.class);
ids.put(121, org.elasticsearch.search.aggregations.InvalidAggregationPathException.class);
ids.put(122, null);
ids.put(123, org.elasticsearch.indices.IndexAlreadyExistsException.class);

This comment has been minimized.

@s1monw

s1monw Nov 16, 2016

Contributor

this doesn't work sorry... you can't add a new exception here it's a wire protocol break. Just reuse the 123 and add a comment that we renamed it

@s1monw

s1monw Nov 16, 2016

Contributor

this doesn't work sorry... you can't add a new exception here it's a wire protocol break. Just reuse the 123 and add a comment that we renamed it

This comment has been minimized.

@dimitris-athanasiou

dimitris-athanasiou Nov 16, 2016

Contributor

Good to know! Will change ASAP.

@dimitris-athanasiou

dimitris-athanasiou Nov 16, 2016

Contributor

Good to know! Will change ASAP.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Nov 16, 2016

Contributor

for the backport I think we need to add a BWC layer here other than that this change LGTM...

Contributor

s1monw commented Nov 16, 2016

for the backport I think we need to add a BWC layer here other than that this change LGTM...

@dimitris-athanasiou

This comment has been minimized.

Show comment
Hide comment
@dimitris-athanasiou

dimitris-athanasiou Nov 16, 2016

Contributor

I opened a backport PR at #21601.

Contributor

dimitris-athanasiou commented Nov 16, 2016

I opened a backport PR at #21601.

@dimitris-athanasiou dimitris-athanasiou merged commit a75320f into elastic:master Nov 17, 2016

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@lcawl lcawl added :Core/Core and removed :Exceptions labels Feb 13, 2018

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