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.

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

Use a dedicated id to serialize EsExceptions instead of it's class name. #13629

Merged
merged 1 commit into from Sep 17, 2015

Conversation

@s1monw
Copy link
Contributor

s1monw commented Sep 17, 2015

Classnames change quickly due to refactorings etc. If that happens in a minor release
we loose the ability to deserialize the exceptoin coming from another node sicne we today
look it up by classname. This change uses a dedicated static id instead of the classname
to lookup the actual class.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

s1monw commented Sep 17, 2015

since the exception mechanism is new in 2.0 I think this should go into 2.0GA since it prevents subtile bugs and is a minor change.

@bleskes
bleskes reviewed Sep 17, 2015
View changes
core/src/main/java/org/elasticsearch/ElasticsearchException.java Outdated

final int maxOrd = 140;
assert exceptions.size() == maxOrd + 1;
Constructor<? extends ElasticsearchException>[] mapping = new Constructor[maxOrd + 1];

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 17, 2015

Member

can we call this idToClass?

@bleskes
bleskes reviewed Sep 17, 2015
View changes
core/src/main/java/org/elasticsearch/ElasticsearchException.java Outdated
}
mapping.put(name, constructor);
if (mapping[e.getValue().intValue()] != null) {

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 17, 2015

Member

can we check that intValue is non-negative? (we use it in vint)

@s1monw

This comment has been minimized.

Copy link
Contributor Author

s1monw commented Sep 17, 2015

@bleskes applied review comments

@bleskes

This comment has been minimized.

Copy link
Member

bleskes commented Sep 17, 2015

LGTM.

Classnames change quickly due to refactorings etc. If that happens in a minor release
we loose the ability to deserialize the exceptoin coming from another node sicne we today
look it up by classname. This change uses a dedicated static id instead of the classname
to lookup the actual class.
@s1monw s1monw force-pushed the s1monw:fix_exception_serialization branch to af9166d Sep 17, 2015
@s1monw s1monw merged commit af9166d into elastic:master Sep 17, 2015
1 check passed
1 check passed
CLA Commit author is a member of Elasticsearch
Details
@s1monw s1monw deleted the s1monw:fix_exception_serialization branch Sep 17, 2015
@clintongormley clintongormley added v2.0.0-rc1 and removed v2.0.0 labels Oct 7, 2015
@lcawl lcawl added :Core/Infra/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
Projects
None yet
4 participants
You can’t perform that action at this time.