Elastisch should use actionGet not get

[richievos]

In one of our apps, we've built our own elastic search client, that we've recently been attempting to replace with elastisch. Overall this has gone pretty well, but we've had a couple issues.

The most recent one is we are handling some errors elastic search throws in our code, and seeing different behavior now that we have moved to elastisch. After digging through things a bit, it seems to be caused by elastisch doing:

 (.get response)

versus

 (.actionGet response)

While this normally isn't noticeable, the primary reason actionGet exists is to wrap the exceptions that happen, and make sure elasticsearch variants get thrown, so for those of us that care about the exceptions, it'd be preferential to use actionGet.

This is also why most examples on the web are using actionGet.

Unless there was a specific reason elastisch uses get, I'd be happy to send a PR fixing this, but wanted to check first.

For reference, actionGet is implemented in https://github.com/elasticsearch/elasticsearch/blob/master/src/main/java/org/elasticsearch/action/support/AdapterActionFuture.java

@Override
public T actionGet() throws ElasticsearchException {
    try {
        return get();
    } catch (InterruptedException e) {
        throw new ElasticsearchInterruptedException(e.getMessage());
    } catch (ExecutionException e) {
        throw rethrowExecutionException(e);
    }
}

[richievos]

Also, to be specific, the methods I'm specifically referring to are clojurewerkz.elastisch.native.document/create and clojurewerkz.elastisch.native.document/put

https://github.com/clojurewerkz/elastisch/blob/master/src/clojurewerkz/elastisch/native/document.clj#L47

[michaelklishin]

OK, makes sense. Feel free to submit a pull request!

[richievos]

@michaelklishin see #57 for an initial pass at this. Everything works, but I didn't add any tests specific to this. I'm honestly a little conflicted about how to do that / if it should be.

The correct behavior to test is that the proper exceptions get thrown in some situations (like version errors). But there's places all over that were switched to actionGet, and testing all of them seems a bit off. And at that point it's kind of just testing that the method elastisch is calling is the method ES generally recommends for this these days.

So long story short, in my head I was able to justify not adding additional tests to myself. If you disagree, feel free to say so, and I'll try thinking it through again.

[michaelklishin]

LGTM. There is no need to test all actions, pick a couple of common cases (e.g. when an index does not exist) and add tests for them. Thank you!

[richievos]

Cool. I'll test this indirectly as part of #56, since the proper versioning exception is only thrown with the actionGet call