Java API improvements


(Shay Banon) #1

Two important improvements just pushed to master (will be part of 0.18) that
are there to correct confusing behavior with the Java API:

  1. There is no more thread based caching of XContentBuilder, some have been
    bitten by it, and its not worth the trouble it created. Issue:
    https://github.com/elasticsearch/elasticsearch/issues/1291.

  2. When working with ActionFuture (getting one from execute on the relevant
    prepareXXX methods), actionGet will now throw the unwrapped / action
    exception, and not a possibly wrapped one. Issue:
    https://github.com/elasticsearch/elasticsearch/issues/1292.

-shay.banon


(Paul Brown) #2

On Aug 31, 2011, at 2:25 PM, Shay Banon wrote:

Two important improvements just pushed to master (will be part of 0.18) that are there to correct confusing behavior with the Java API:

  1. There is no more thread based caching of XContentBuilder, some have been bitten by it, and its not worth the trouble it created. Issue: https://github.com/elasticsearch/elasticsearch/issues/1291.

  2. When working with ActionFuture (getting one from execute on the relevant prepareXXX methods), actionGet will now throw the unwrapped / action exception, and not a possibly wrapped one. Issue: https://github.com/elasticsearch/elasticsearch/issues/1292.

Speaking as a user of the Java API, #2 is a very welcome change, and it would be great if the specific exceptions that can be thrown (versus the general ElasticSearchException) are declared on the methods. The convenience method to determine whether the root cause is a particular type is useful, but it still has sent me reading through the Elasticsearch code to find out what exceptions may be thrown under which circumstances.

In terms of other API clean-up suitable for a version jump, there are some other situations where some builder-style methods can return null (I'm thinking specifically of accessing admin information about indexes), and it would be great to clean that up as well. (My main objection to builder-style APIs is not to the builder-style itself but rather that people often break the contract of non-null return values…)

-- Paul


(Shay Banon) #3

On Thu, Sep 1, 2011 at 12:35 AM, Paul Brown prb@mult.ifario.us wrote:

On Aug 31, 2011, at 2:25 PM, Shay Banon wrote:

Two important improvements just pushed to master (will be part of 0.18)
that are there to correct confusing behavior with the Java API:

  1. There is no more thread based caching of XContentBuilder, some have been
    bitten by it, and its not worth the trouble it created. Issue:
    https://github.com/elasticsearch/elasticsearch/issues/1291.

  2. When working with ActionFuture (getting one from execute on the relevant
    prepareXXX methods), actionGet will now throw the unwrapped / action
    exception, and not a possibly wrapped one. Issue:
    https://github.com/elasticsearch/elasticsearch/issues/1292.

Speaking as a user of the Java API, #2 is a very welcome change, and it
would be great if the specific exceptions that can be thrown (versus the
general ElasticSearchException) are declared on the methods. The
convenience method to determine whether the root cause is a particular type
is useful, but it still has sent me reading through the Elasticsearch code
to find out what exceptions may be thrown under which circumstances.

This is more of a documentation question, you can't list all the possible
exception for different APIs on the same "ActionFuture".

In terms of other API clean-up suitable for a version jump, there are some
other situations where some builder-style methods can return null (I'm
thinking specifically of accessing admin information about indexes), and it
would be great to clean that up as well. (My main objection to
builder-style APIs is not to the builder-style itself but rather that people
often break the contract of non-null return values…)

I did not follow you, maybe a concrete example?

-- Paul


(Paul Brown) #4

Hi, Shay --

I agree on the documentation side as a minimum for the exceptions, although even that is a bit painful from a developer's perspective. (The compiler can do the work if the exceptions are declared.) It would be useful to be able to raise an exception (perhaps as a matter of policy, perhaps as the default behavior) if there are any shard failures or failures to acknowledge.

For the (purported) builder API issues, I went back over the code, and it looks like the only null guarding that was needed was for the aliases for an index, and the possibly null return value there is fair to expect of the developer because it's a get on a map.

-- Paul

On Aug 31, 2011, at 2:44 PM, Shay Banon wrote:

On Thu, Sep 1, 2011 at 12:35 AM, Paul Brown prb@mult.ifario.us wrote:
On Aug 31, 2011, at 2:25 PM, Shay Banon wrote:

Two important improvements just pushed to master (will be part of 0.18) that are there to correct confusing behavior with the Java API:

  1. There is no more thread based caching of XContentBuilder, some have been bitten by it, and its not worth the trouble it created. Issue: https://github.com/elasticsearch/elasticsearch/issues/1291.

  2. When working with ActionFuture (getting one from execute on the relevant prepareXXX methods), actionGet will now throw the unwrapped / action exception, and not a possibly wrapped one. Issue: https://github.com/elasticsearch/elasticsearch/issues/1292.

Speaking as a user of the Java API, #2 is a very welcome change, and it would be great if the specific exceptions that can be thrown (versus the general ElasticSearchException) are declared on the methods. The convenience method to determine whether the root cause is a particular type is useful, but it still has sent me reading through the Elasticsearch code to find out what exceptions may be thrown under which circumstances.

This is more of a documentation question, you can't list all the possible exception for different APIs on the same "ActionFuture".

In terms of other API clean-up suitable for a version jump, there are some other situations where some builder-style methods can return null (I'm thinking specifically of accessing admin information about indexes), and it would be great to clean that up as well. (My main objection to builder-style APIs is not to the builder-style itself but rather that people often break the contract of non-null return values…)

I did not follow you, maybe a concrete example?

-- Paul


(system) #5