[GitHub] [geode] mcmellawatt opened pull request #2484: GEODE-5747: Handling SocketException in InternalDataSerializer

["mcmellawatt (GitHub)" <gi...@apache.org>]

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:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

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

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

- [X] Does `gradlew build` run cleanly?

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

- [X] 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/2484 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt closed pull request #2484: GEODE-5747: Handling SocketException in InternalDataSerializer

["mcmellawatt (GitHub)" <gi...@apache.org>]

[ pull request closed by mcmellawatt ]

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2484: GEODE-5747: Handling SocketException in InternalDataSerializer

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

I see callers of this method that have their own try/catch that will turn this SocketException into a SerializationException and then throw it. I could not tell what the context was of the code that will retry if it sees a SocketException but it seems like some of these other code paths would be possible.
Instead of changing all the places that currently throw SerializationException, did you consider changing the gateway sender code to handle a SerializationException by checking its cause and if it is a SocketException retrying?

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2484: GEODE-5747: Handling SocketException in InternalDataSerializer

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

If you look at: InternalDataSerializer.readDataSerializable(DataInput) you will see it calls your modified invokeFromData but turns the SocketException into a SerializationException. So my only  concern is that you will need to change other places to treat SocketException like an EOFException.  Same goes for readDataSerializableFixedID except it currently also transforms EOFException. invokeFromData has lots of other callers but I don't know if any of them have a catch that rethrows. I'm okay with you treating a SocketException like EOFException just wanted to make sure you covered the other code paths and thought it might be easier to handle the SerializationException. But I'll defer to you to pick which is best.

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

[GitHub] [geode] galen-pivotal commented on pull request #2484: GEODE-5747: Handling SocketException in InternalDataSerializer

["galen-pivotal (GitHub)" <gi...@apache.org>]

Is there a reason not to catch `IOException` more generally?

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

[GitHub] [geode] mcmellawatt commented on pull request #2484: GEODE-5747: Handling SocketException in InternalDataSerializer

["mcmellawatt (GitHub)" <gi...@apache.org>]

Thanks for digging deep into this.  I think it is somewhat concerning that the exception handling is so variable across these different methods - for instance, handling EOFException in readDataSerializable(DataInput) and not handling it in readDataSerializableFixedID(DataInput).  

I think its probably worth determining what happens in all of the callers if the SocketException is wrapped in a SerializationException (as it would be without this PR's changes).  If other components are similar to the gateway sender in that they treat SerializationExceptions as fatal but IOExceptions as non-fatal, then we may see other problems arise.  We'll spend some more time looking at this.  Thanks again!

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

[GitHub] [geode] mcmellawatt commented on pull request #2484: GEODE-5747: Handling SocketException in InternalDataSerializer

["mcmellawatt (GitHub)" <gi...@apache.org>]

We did consider a variation of what you are suggesting, but fundamentally we saw a SocketException as an indication that the connection to the client was in a bad state and figured we should treat it the same as an EOFException.

In the context of a gateway sender, the SerializationException that was wrapping the SocketException would cause all event processing to stop, which is undesirable.  There is handling for all IOExceptions (including SocketExceptions) in the gateway sender, so that it attempts to connect to a different locator instead of shutting down the event processor.  As you suggested, we could add handling at that part of the code to catch SerializationExceptions, check if the inner exception is a SocketException, and retry.  Do you prefer that option?  I assume you may prefer it because the scope of the change is minimized.  

Again, our thought for the change in this PR is that a SocketException is similar to the EOFException in that it indicates a bad connection, and callers have their own handling for these exception types (i.e. retrying in the gateway sender case).  We spent some time looking at various callers and didn't see any red flags for this change.  I could be convinced to instead make the change at the gateway sender logic if there is fear of undesired side effects though.  Let me know.

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

[GitHub] [geode] mcmellawatt commented on pull request #2484: GEODE-5747: Handling SocketException in InternalDataSerializer

["mcmellawatt (GitHub)" <gi...@apache.org>]

We did consider a variation of what you are suggesting, but fundamentally we saw a SocketException as an indication that the connection to the client was in a bad state and figured we should treat it the same as an EOFException.

In the context of a gateway sender, the SerializationException that was wrapping the SocketException would cause all event processing to stop, which is undesirable.  There is handling for all IOExceptions (including SocketExceptions) in the gateway sender, so that it attempts to connect to a different locator instead of shutting down the event processor.  As you suggested, we could add handling at that part of the code to catch SerializationExceptions, check if the inner exception is a SocketException, and retry.  Do you prefer that options?  I assume you may prefer it because the scope of the change is minimized.  

Again, our thought for the change in this PR is that a SocketException is similar to the EOFException in that it indicates a bad connection, and callers have their own handling for these exception types (i.e. retrying in the gateway sender case).  We spent some time looking at various callers and didn't see any red flags for this change.  I could be convinced to instead make the change at the gateway sender logic if there is fear of undesired side effects though.  Let me know.

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