You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "jhuynh1 (GitHub)" <gi...@apache.org> on 2018/11/05 22:45:47 UTC

[GitHub] [geode] jhuynh1 opened pull request #2786: GEODE-5884: Reverts behavior of exceptions/wrapping of exceptions

  * A previous refactor in a5daa924 was causing behavoural changes
    with function exceptions that GEODE-5884 was exposing.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [ ] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/2786 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jhuynh1 commented on pull request #2786: GEODE-5884: Reverts behavior of exceptions/wrapping of exceptions

Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.

I don't think we should have to, but the product and customers and specific tests were coded to look for the causes() of a functionException and decide whether it should retry or not.  Instead the callers should have been able to just catch the specific exceptions instead of drilling into nested causes. Maybe we can do that for Geode 2.0.

InternalFunctionInvocationTargetException should not be handed back to the caller of the function service.  Those are used internally to decide whether to retry a function or not.  So I can understand not wanting to send those back to a client.

[ Full content available at: https://github.com/apache/geode/pull/2786 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2786: GEODE-5884: Reverts behavior of exceptions/wrapping of exceptions

Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.

Can you explain conceptually why we need to wrap these different exception types in a new FunctionException?   Does it have repercussions in other parts of the code if we leave them in their original form (FunctionInvocationTargetException, InternalFunctionInvocationTargetException)?

[ Full content available at: https://github.com/apache/geode/pull/2786 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2786: GEODE-5884: Reverts behavior of exceptions/wrapping of exceptions

Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.

Isn't this addException logic the same in all three conditional branches?  Seems like this could be extracted out of the conditional and called once at the bottom of this method.

[ Full content available at: https://github.com/apache/geode/pull/2786 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jhuynh1 closed pull request #2786: GEODE-5884: Reverts behavior of exceptions/wrapping of exceptions

Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.

[ pull request closed by jhuynh1 ]

[ Full content available at: https://github.com/apache/geode/pull/2786 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2786: GEODE-5884: Reverts behavior of exceptions/wrapping of exceptions

Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.

Nevermind, I now see that this.functionException changes in each branch.

[ Full content available at: https://github.com/apache/geode/pull/2786 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org