[BEAM-3279] Deprecate and remove Coder.consistentWithEquals by AlexKbit · Pull Request #8071 · apache/beam

AlexKbit · 2019-03-15T15:10:10Z

Hi @kennknowles !
Could you please review my changes:

Deprecate Coder.consistentWithEquals method and add javadoc
Remove overriding from coders in the java-core

I'm not sure about removal of overriding from :
org.apache.beam.sdk.coders.MapCoder#consistentWithEquals
org.apache.beam.sdk.coders.ListCoder#consistentWithEquals

because it breaks the tests below:
org.apache.beam.sdk.coders.MapCoderTest > testConsistentWithEquals FAILED
org.apache.beam.sdk.coders.ListCoderTest > testConsistentWithEquals FAILED

I can rewrite these tests if it's necessary.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go	---	---	---	---	---	---
Java
Python	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

kennknowles

Thanks for working on this!

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/BigEndianShortCoder.java

kennknowles

Great. This looks like a good improvement.

sdks/java/core/src/test/java/org/apache/beam/sdk/coders/ListCoderTest.java

AlexKbit · 2019-03-24T20:48:10Z

@kennknowles this change will ready to merge, If you don't have other comments

…g in core

AlexKbit · 2019-03-25T09:07:49Z

@kennknowles PR squashed & ready to merge.
Thank you for your time!

kennknowles · 2019-03-25T17:24:21Z

@AlexKbit I opened a pull request to you for minor changes. Did you see it?

AlexKbit · 2019-03-25T17:41:26Z

@kennknowles Do you mean tests with hardcoded expected result for consistentWithEquals?
Yep. I removed tests from ListCoderTest & MapCoderTest.

kennknowles · 2019-03-25T17:51:31Z

https://github.com/AlexKbit/beam/pull/1

[BEAM-3279] Deprecate and remove Coder.consistentWithEquals

AlexKbit · 2019-03-25T19:08:42Z

@kennknowles should i do squash with this changes again?

kennknowles · 2019-03-25T20:37:31Z

I can squash them for you. Thanks!

kennknowles · 2019-03-25T20:39:50Z

Thanks! If you want to follow-up there is good test coverage missing here: each coder could have a test, shared in CoderProperties, that the equality on encoded bytes matches the equality on structuralValue.

AlexKbit · 2019-03-25T20:46:11Z

Thank you! Can you create another ticket for this task(assign to me)?

kennknowles · 2019-03-25T20:54:36Z

https://issues.apache.org/jira/browse/BEAM-6904

AlexKbit · 2019-03-25T20:56:30Z

Thank you! 😄

)

kennknowles · 2019-04-09T15:39:34Z

Congratulations! This change (which is good) exposed a critical bug in the core SDK. We may have to roll this back, hopefully only temporarily, while we figure out what to do.

…pache#8071)" This reverts commit 43db80d.

[BEAM-7038] Revert "[BEAM-3279] Deprecate and remove Coder.consistentWithEquals (#8071)

[BEAM-7038] Revert "[BEAM-3279] Deprecate and remove Coder.consistentWithEquals (apache#8071)

…pache#8071)" This reverts commit 43db80d.

kennknowles · 2019-06-10T16:05:24Z

To roll this forward, we need to do a few things:

understand which core transforms are affected when the serialized coder changes
preferred: replace all Java serialized coders in the core SDK with StructuredCoder
- for runners like Dataflow (and I think Flink) that do in-place update, make them OK with the change

AlexKbit added 2 commits March 15, 2019 17:53

[BEAM-3279] Deprecate and remove Coder.consistentWithEquals

797b29b

[BEAM-3279] revert changes for coders

e724b1d

kennknowles self-requested a review March 15, 2019 23:00

kennknowles requested changes Mar 15, 2019

View reviewed changes

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/BigEndianShortCoder.java Show resolved Hide resolved

[BEAM-3279] override structure value and add unit tests

a2d0865

kennknowles requested changes Mar 22, 2019

View reviewed changes

sdks/java/core/src/test/java/org/apache/beam/sdk/coders/ListCoderTest.java Outdated Show resolved Hide resolved

[BEAM-3279] remove consistentWithEquals tests

a22d043

kennknowles approved these changes Mar 25, 2019

View reviewed changes

kennknowles and others added 2 commits March 24, 2019 21:02

fixup! [BEAM-3279] Deprecate and remove Coder.consistentWithEquals

a6866c9

[BEAM-3279] Deprecate Coder.consistentWithEquals and remove overridin…

5b8ffae

…g in core

AlexKbit added 2 commits March 25, 2019 20:56

Merge branch 'feature/BEAM-3279' into wording

bebc94e

Merge pull request #1 from kennknowles/wording

3292135

[BEAM-3279] Deprecate and remove Coder.consistentWithEquals

kennknowles merged commit 43db80d into apache:master Mar 25, 2019

AlexKbit deleted the feature/BEAM-3279 branch March 26, 2019 07:42

RobbeSneyders pushed a commit to RobbeSneyders/beam that referenced this pull request Apr 9, 2019

[BEAM-3279] Deprecate and remove Coder.consistentWithEquals (apache#8071

d6b5494

)

aaltay added a commit to aaltay/beam that referenced this pull request Apr 9, 2019

Revert "[BEAM-3279] Deprecate and remove Coder.consistentWithEquals (a…

f5029b4

…pache#8071)" This reverts commit 43db80d.

aaltay mentioned this pull request Apr 9, 2019

[BEAM-7038] Revert "[BEAM-3279] Deprecate and remove Coder.consistentWithEquals (#8071) #8258

Merged

3 tasks

aaltay added a commit that referenced this pull request Apr 9, 2019

Merge pull request #8258 from aaltay/j7038

bcebd8b

[BEAM-7038] Revert "[BEAM-3279] Deprecate and remove Coder.consistentWithEquals (#8071)

aaltay added a commit to aaltay/beam that referenced this pull request Apr 9, 2019

Merge pull request apache#8258 from aaltay/j7038

6339847

[BEAM-7038] Revert "[BEAM-3279] Deprecate and remove Coder.consistentWithEquals (apache#8071)

aaltay mentioned this pull request Apr 9, 2019

[BEAM-7038] Merge pull request #8258 to release branch #8265

Merged

3 tasks

ibzib pushed a commit to ibzib/beam that referenced this pull request Apr 22, 2019

Revert "[BEAM-3279] Deprecate and remove Coder.consistentWithEquals (a…

1eba85f

…pache#8071)" This reverts commit 43db80d.

AlexKbit mentioned this pull request Jun 8, 2019

[BEAM-6904] Add tests for structuralValue implementation in coders #8208

Closed

Conversation

AlexKbit commented Mar 15, 2019

Post-Commit Tests Status (on master branch)

Uh oh!

kennknowles left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kennknowles left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexKbit commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexKbit commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennknowles commented Mar 25, 2019

Uh oh!

AlexKbit commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennknowles commented Mar 25, 2019

Uh oh!

AlexKbit commented Mar 25, 2019

Uh oh!

kennknowles commented Mar 25, 2019

Uh oh!

kennknowles commented Mar 25, 2019

Uh oh!

AlexKbit commented Mar 25, 2019

Uh oh!

kennknowles commented Mar 25, 2019

Uh oh!

AlexKbit commented Mar 25, 2019

Uh oh!

kennknowles commented Apr 9, 2019

Uh oh!

kennknowles commented Jun 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AlexKbit commented Mar 24, 2019 •

edited

Loading

AlexKbit commented Mar 25, 2019 •

edited

Loading

AlexKbit commented Mar 25, 2019 •

edited

Loading