[GitHub] [geode] jinmeiliao opened pull request #2491: GEODE-3: fix analyzeDataSerializable for java9 and above

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

Co-authored-by: Owen Nichols <on...@pivotal.io>

* some optimization in java9 make serialized byte size different

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

[GitHub] [geode] upthewaterspout commented on issue #2491: GEODE-3: fix analyzeDataSerializable for java9 and above

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

It's not clear to me how a developer is supposed to maintain these lists going forward. Now if I touch any of these classes I have to run this test with two different versions of Java? Since this test seems to be here to fail whenever the developer changes one of these methods, maybe we should just skip the test for java 9 and above?

An alternative would be to stop doing this bytecode comparison and actually execute the toData and fromData methods. There is some complication there in how we populate the object in the first place, but if we actually put the work in we might be able to have much better tests of serialization compatibility.

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

[GitHub] [geode] PurelyApplied commented on issue #2491: GEODE-3: fix analyzeDataSerializable for java9 and above

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

I agree with @kohlmu-pivotal ; we currently have 13 commits on `develop` under GEODE-3.  I favor creating more narrowly focused tickets as sub-tasks to GEODE-3 to cover the scope of a given ticket.  Looking at any of this in hindsight will become much more difficult when so many commits collide to the same ticket.

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

[GitHub] [geode] metatype commented on issue #2491: GEODE-3: fix analyzeDataSerializable for java9 and above

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

I agree with @upthewaterspout.  If I wonder if there is a better way to notice that serialization of a class has changed w/o needing to run tests on multiple JVM's.

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

[GitHub] [geode] PurelyApplied closed pull request #2491: GEODE-5815: ignore testDataSerializable for java9 and above

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

[ pull request closed by PurelyApplied ]

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

[GitHub] [geode] jinmeiliao commented on issue #2491: GEODE-3: fix analyzeDataSerializable for java9 and above

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

I think it boils down to what exactly this test is supposed to test. This PR is to prompt the thinking of how we test data serialization.  I admit this is just a hack to make test pass.

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

[GitHub] [geode] pivotal-jbarrett commented on issue #2491: GEODE-3: fix analyzeDataSerializable for java9 and above

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

How about we stop relying on Java serialization already?

> On Sep 21, 2018, at 3:22 PM, Anthony Baker <no...@github.com> wrote:
> 
> I agree with @upthewaterspout. If I wonder if there is a better way to notice that serialization of a class has changed w/o needing to run tests on multiple JVM's.
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or mute the thread.


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