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

move ignore parameter support from yaml test client to low level rest client #22637

Merged
merged 4 commits into from Jan 16, 2017

Conversation

Projects
None yet
2 participants
@javanna
Member

javanna commented Jan 16, 2017

All the language clients support a special ignore parameter that doesn't get passed to elasticsearch with the request, but is used to indicate which error code(s) should not lead to an exception if returned for a specific request.

Moving this to the low level REST client will allow the high level REST client to make use of it too, for instance so that it doesn't have to intercept ResponseExceptions when the get api returns a 404, which is in that specific case a perfectly valid response code (e.g. the body returned is not an exception in that case).

move ignore parameter support from yaml test client to low level rest…
… client

All the language clients support a special ignore parameter that doesn't get passed to elasticsearch with the request, but used to indicate which error code should not lead to an exception if returned for a specific request.

Moving this to the low level REST client will allow the high level REST client to make use of it too, for instance so that it doesn't have to intercept ResponseExceptions when the get api returns a 404.
@javanna

This comment has been minimized.

Member

javanna commented Jan 16, 2017

@nik9000 can you have a look please?

@s1monw

left some comments

URI uri = buildUri(pathPrefix, endpoint, params);
Objects.requireNonNull(params, "params must not be null");
Map<String, String> requestParams = new HashMap<>(params);
Set<Integer> ignoreErrorCodes = new HashSet<>();

This comment has been minimized.

@s1monw

s1monw Jan 16, 2017

Contributor

should we initialize this conditionally and if ignoreString == null we just use Collections.emptySet()?

@@ -59,7 +57,7 @@
* Query params that don't need to be declared in the spec, they are supported by default.
*/
private static final Set<String> ALWAYS_ACCEPTED_QUERY_STRING_PARAMS = Sets.newHashSet(
"error_trace", "filter_path", "human", "pretty", "source");
"ignore", "error_trace", "human", "filter_path", "pretty", "source");

This comment has been minimized.

@s1monw

s1monw Jan 16, 2017

Contributor

now that we support this should we document it somewhere?

ignoreErrorCodes.add(404);
}
//ignore is a special parameter supported by the clients, shouldn't be sent to es
String ignoreString = requestParams.remove("ignore");

This comment has been minimized.

@s1monw

s1monw Jan 16, 2017

Contributor

does this need javadocs?

@javanna

This comment has been minimized.

Member

javanna commented Jan 16, 2017

@s1monw thanks for the review, I addressed your comments

javanna added some commits Jan 16, 2017

@s1monw

s1monw approved these changes Jan 16, 2017

LGTM

@javanna javanna merged commit 1931119 into elastic:master Jan 16, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

javanna added a commit to javanna/elasticsearch that referenced this pull request Jan 16, 2017

move ignore parameter support from yaml test client to low level rest…
… client (elastic#22637)

All the language clients support a special ignore parameter that doesn't get passed to elasticsearch with the request, but used to indicate which error code should not lead to an exception if returned for a specific request.

Moving this to the low level REST client will allow the high level REST client to make use of it too, for instance so that it doesn't have to intercept ResponseExceptions when the get api returns a 404.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 16, 2017

Merge branch 'master' into replica-sequence-number-recovery
* master: (131 commits)
  Replace EngineClosedException with AlreadyClosedExcpetion (elastic#22631)
  Remove HttpServer and HttpServerAdapter in favor of a simple dispatch method (elastic#22636)
  move ignore parameter support from yaml test client to low level rest client (elastic#22637)
  Fix thread safety of Stempel's token filter factory (elastic#22610)
  Update profile.asciidoc
  Wrap rest httpclient with doPrivileged blocks (elastic#22603)
  Revert "Add a deprecation notice to shadow replicas (elastic#22025)"
  Revert "Don'y use `INDEX_SHARED_FS_ALLOW_RECOVERY_ON_ANY_NODE_SETTING` directly as it triggers (many) deprecation logging"
  Don'y use `INDEX_SHARED_FS_ALLOW_RECOVERY_ON_ANY_NODE_SETTING` directly as it triggers (many) deprecation logging
  Add a deprecation notice to shadow replicas (elastic#22025)
  [Docs] Fix section title in profile.asciidoc
  ProfileResult and CollectorResult should print machine readable timing information (elastic#22561)
  Add replica ops with version conflict to translog
  Replace custom Functional interface in ElasticsearchException with CheckedFunction
  Make RestChannelConsumer extend CheckedConsumer<RestChannel, Exception>
  replace ShardSearchRequest.FilterParser functional interface with CheckedFunction
  replace custom functional interface with CheckedFunction in percolate module
  [TEST] replace SizeFunction with Function<Integer, Integer>
  Expose CheckedFunction
  Expose logs base path
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment