Add support for create actions in the node client

[pickypg]

• Adds validation to require _id for create actions (including for the http protocol)

Help: I don't know how to properly test for the error condition. The "create action fails without _id" test fails because I'm not testing it right, but I don't know how to properly catch the error.

Note: #2 (retry on bulk failure) may need to special case failure to create (ignore it).

Closes #28

[talevy]

@pickypg looks good! Until we merge #2, I do not see how to verify failure well. The current behavior of the elasticsearch output is to drop messages that failed in bulk because the response is not checked for failures.

is this an urgent change?

[pickypg]

@talevy I'd like to get the change in and honestly I think people expect the current behavior from create (drop and ignore).

The integration tests definitely do not behave that way though. As I have it, the "create action fails without _id" test always fails and it seems to loop trying to resend the action (I want to have a test, but I didn't know how to stop the resending behavior). Should I just drop that test and merge this?

[jordansissel]

@pickypg Any behavior that results in "drop on error" should be very loudly documented. Are you proposing that create-with-an-id-that-already-exists should result in a dropped event?

[pickypg]

@jordansissel Yes. In the logging use case, selecting this is very manual and it seems quite intentional. If you don't want it, then leaving it as "index" (default) would suffice.

[jordansissel]

I wonder if we should make it more explicit, call it create_unless_exists ?

[pickypg]

I'm fine with modifying it that way, but that means we have to further special case it for the non-node clients. Then I suppose we could separately have "create" as-is (though retry on bulk failure may get clumsy here as it's unlikely to ever be successful unless an earlier delete had failed) without the ID validation.

[jordansissel]

@pickypg yeah, I'm less worried about the specific implementation details (special-casing) and more concerned with having an interface and terminology that is most-obvious (nobody will read docs to know what Elasticsearch's create really means).

[pickypg]

@jordansissel Fine by me. I'll find time to update the PR. Do you have any thoughts on how to appropriately test it? It's my lack of Ruby knowledge doing me in, but I'd like to see this make it in.

[jordansissel]

@pickypg yep! We'll get someone helping you out with that this week or next. This should be doable for 1.5's timeline

[jordansissel]

Thinking more outloud - Once we have @colinsurprenant's persistent-queue work merged as well as the dead-letter-queueing, we could just have this be "create" and a failure would dump into the DLQ.

For 1.5, I think 'create_unless_exists' is what to do. For 2.0, we can add 'create' also but shunting errors to the DLQ.

[suyograo]

testing for failures requires changes in #2 to get in.

[suyograo]

@pickypg @talevy wanted to rebase this after #2 got merged in

[suyograo]

Closed in lieu of #39