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

Introduced a new elasticsearch exception family that can hold headers #7269

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@uboness
Copy link
Contributor

uboness commented Aug 14, 2014

  • These headers will be copied as response header on the rest response

@uboness uboness added v2.0.0 labels Aug 14, 2014

@spinscale

This comment has been minimized.

Copy link
Member

spinscale commented Aug 14, 2014

Why not doing an own interface like Headerizable here, that any exception can possibly implement (still working on my naming skills, but you get the point)?

@uboness

This comment has been minimized.

Copy link
Contributor Author

uboness commented Aug 14, 2014

@spinscale thought about it as well (though I must say the name Headerizable never crossed my mind :D)... had it initially, then removed it, I can add it again, but I still want to keep the WithHeaders construct (that can implement HasHeaders)

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 14, 2014

What about having it on the ElasticsearchException class directly?

@uboness

This comment has been minimized.

Copy link
Contributor Author

uboness commented Aug 14, 2014

@jpountz I'm afraid it'll break serialization bwc, but also I'm not sure every es exception should have it

@jpountz

View changes

src/main/java/org/elasticsearch/rest/HasRestHeaders.java Outdated
public interface HasRestHeaders {

Map<String, List<String>> getHeaders();
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 14, 2014

Contributor

Can you add docs to this class?

This comment has been minimized.

Copy link
@uboness

uboness Aug 14, 2014

Author Contributor

I think I can 🎱

@jpountz

View changes

src/main/java/org/elasticsearch/rest/RestResponse.java Outdated
if (customHeaders == null) {
customHeaders = Maps.newHashMap(headers);
return;
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 14, 2014

Contributor

I'm concerned that in this case, the per-key list of headers will not be cloned. Maybe this optimization should be removed and just be:

if (customHeaders == null) {
  customHeaders = Maps.newHashMap(headers.size());
}

This comment has been minimized.

Copy link
@uboness

uboness Aug 14, 2014

Author Contributor

agree

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 14, 2014

OK, good points. I left some comments but other than that it looks good to me.

@jpountz jpountz removed the review label Aug 14, 2014

Introduced a new elasticsearch exception family that can hold headers
 - These heades will be copied as response header on the rest response
@uboness

This comment has been minimized.

Copy link
Contributor Author

uboness commented Aug 14, 2014

updated based on @jpountz comments

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 14, 2014

LGTM

@uboness

This comment has been minimized.

Copy link
Contributor Author

uboness commented Aug 14, 2014

merged by f4a7793

@uboness uboness closed this Aug 14, 2014

@clintongormley clintongormley changed the title Introduced a new elasticsearch exception family that can hold headers Internal: Introduced a new elasticsearch exception family that can hold headers Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Introduced a new elasticsearch exception family that can hold headers Introduced a new elasticsearch exception family that can hold headers Jun 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
You can’t perform that action at this time.