[GitHub] [geode-native] mmartell opened pull request #408: Geode-5962: Fix putAll crash with null values

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

This adds an error handler to TcrMessage to catch PUT_DATA_ERROR messages from the server. All adds a test to ensure a proper exception is bubbled up to the user.

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

[GitHub] [geode-native] mreddington commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

Done.

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

[GitHub] [geode-native] echobravopapa commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

My comment had to do with the claim that `GET_ALL_DATA_ERROR`s are handled at a higher level (line 864) and how does this compare to the route taken for `PUT_DATA_ERROR`? i.e. which is correct as they seem to be handled very differently...

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

[GitHub] [geode-native] mmartell commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

Good catch. Replaced with EXPECT_THROW.

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

[GitHub] [geode-native] mmartell commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

Done.

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

[GitHub] [geode-native] mmartell commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

Done.

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

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

Use `ASSERT_EXCEPTION` https://github.com/abseil/googletest/blob/master/googletest/docs/advanced.md#exception-assertions

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

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

Does this add value to this particular test?

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

[GitHub] [geode-native] echobravopapa commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

`TcrMessage::` ?  for consistency if nothing else

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

[GitHub] [geode-native] mreddington commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

GET_ALL_DATA_ERROR is not a catchall for data errors, it is a specific error response to a get-all data request.

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

[GitHub] [geode-native] pivotal-jbarrett commented on issue #408: Geode-5962: Fix putAll crash with null values

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

`// TODO error handling`

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

[GitHub] [geode-native] echobravopapa commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

should be able to use `ASSERT_THROW`, no?

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

[GitHub] [geode-native] moleske commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

@pdxcodemonkey and I followed the chain up and the comment looks like it was there to explain the `break;` statement.  The errors are not handled at a higher level.

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

[GitHub] [geode-native] mreddington commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

This has been removed.

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

[GitHub] [geode-native] echobravopapa commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

These errors claim to be handled at a higher level, is the handling different than `PUT_DATA_ERROR`?

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

[GitHub] [geode-native] pdxcodemonkey closed pull request #408: Geode-5962: Fix putAll crash with null values

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

[ pull request closed by pdxcodemonkey ]

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

[GitHub] [geode-native] echobravopapa commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

@pivotal-jbarrett preferred `ASSERT_EXCEPTION`

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

[GitHub] [geode-native] mmartell commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

If I understand you correctly, you're saying PUT_DATA_ERROR handler should eat these exceptions just like the other handlers. However, it seems like we should bubbling these exceptions up to the user. It looks like these aren't even logged in the client log, which seems to make it even more important to pass these back to the user.

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

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

You should be using Expect exception.

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

[GitHub] [geode-native] echobravopapa commented on pull request #408: Geode-5962: Fix putAll crash with null values

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

nope.  I'm saying maybe we should handle them all the same in a consistent manner...

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