Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/1490#issuecomment-50426601
Thanks for submitting this patch. I agree that we should propagate errors
to senders if errors occurred while processing their messages. The addition of
the `hasError` field in MessageHeader looks fine to me.
It looks like Future supports `onFailure` callbacks, so I wonder if we
should signal errors by having `sendMessageReliably` check whether an error
occurred and signal failure by calling `promise.failed`. Note that an
exception will be thrown if you call `Await.result()` on a future that fails.
Instead of registering an `onSuccess` callback and detecting failures inside of
it, users would have to define an additional `onFailure` callback or define
`onComplete` and pattern-match on `Success` or `Failure`.
Returning a failed future might prevent bugs where users forget to check
the `hasFailed` field, treat the failure message as a valid response, and
experience unexpected behavior; instead, their code would either throw an
exception or their callback would never run (assuming they never defined
`onFailure`).
@rxin Any thoughts on signaling failure through failed futures vs. the
approach in this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
