[GitHub] [geode] Bill opened pull request #4364: GEODE-7344: DSFID implements BasicSerializable

["Bill (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:
- [ ] 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, check Concourse 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/4364 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

Actually we don't need that cast at all since `invokeToData()` takes an `Object`. Fixing…

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

[GitHub] [geode] Bill commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

I understand your point. OTOH we (now) clearly document `BasicSerializable` as an interface that is solving a particular legacy problem and we clearly say not to use it for new work. Inasmuch as only 6 classes implement this interface and only 2/6 need to override the (no-op) behavior, I'm gonna leave the `default` impls in.

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

[GitHub] [geode] dschneider-pivotal commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

You are right. I'm not sure why I thought it was the "context"

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

[GitHub] [geode] dschneider-pivotal commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

I don't like "default" on these methods. It seems better, in this case, to insist that implementors of this interface implement toData and fromData.

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

[GitHub] [geode] dschneider-pivotal commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

Should you explain when this interface should be used?

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

[GitHub] [geode] dschneider-pivotal commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

why rename this to "bs" since it is still a DataSerializable?

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

[GitHub] [geode] Bill commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

I don't think so. The method does write to `DataOutput`.

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

[GitHub] [geode] Bill commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

why does that seem better @dschneider-pivotal ?

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

[GitHub] [geode] dschneider-pivotal commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

The defaults make it too easy to implement this interface and have it serialize nothing. By forcing the implementor to implement toData and fromData they get a chance to consider if their class has any data that needs to be serialized/deserialized. I don't think you have any backwards compatibility to be concerned about so you don't need to use default for that reason. And you don't actually have any interesting "default" implementation here (just empty methods).

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

[GitHub] [geode] Bill commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

yes! changing…

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

[GitHub] [geode] Bill commented on pull request #4364: GEODE-7344: DSFID implements BasicSerializable

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

I don't think so.

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