[BEAM-2021] Make Most StandardCoders CustomCoders by tgroh · Pull Request #2668 · apache/beam

tgroh · 2017-04-24T22:50:29Z

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify. (Even better, enable
Travis-CI on your fork and ensure the whole test matrix passes).
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

Standard Coders have a defined serialization format and are understood
within the Runner API, Custom Coders are not. Move existing
"StandardCoders" to extend CustomCoder, and remove custom cloud object
related serialization logic where possible.

Standard Coders have a defined serialization format and are understood within the Runner API, Custom Coders are not. Move existing "StandardCoders" to extend CustomCoder, and remove custom cloud object related serialization logic where possible. Still remaining: Splitting the CustomCoder side of the class hierarchy from the StandardCoder side of the hierarchy, moving IterableLikeCoder to be a CustomCoder, and have IterableCoder forward to an internal implementation (to ensure it remains a StandardCoder).

tgroh · 2017-04-25T01:30:45Z

R: @lukecwik @kennknowles

kennknowles

What does this improve? Can you clarify what the meaning of the changes are here? Are they just temporary as things are put in place?

As I understand it, all CustomCoder subclasses will share a single coder URN (something like "urn:beam:coder:javasdk") and the payload will contain a Java serialized blob, hence the data will only really be readable by the Java SDK harness. In some sense, this is what "custom coder" means at the level of the portable Beam model.

The other possibility is that a coder has a meaningful URN (like "urn:beam:coder:map") and payload/components determined by that.

So the latter is more meaningful, portable, and compact, just less convenient for one-offs. So I want to understand the rationale for moving things basically from the better case to the worse case. I'm ready to believe it is mandatory for some reason...

kennknowles · 2017-04-25T03:06:29Z

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

 * @param <T> the type of the values being transcoded
 */
-public class LengthPrefixCoder<T> extends StandardCoder<T> {
+public class LengthPrefixCoder<T> extends CustomCoder<T> {


Doesn't this one, in particular, need to have a well-defined URN?

kennknowles · 2017-04-25T03:07:04Z

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

+  }
+
+  @Override
+  public Collection<String> getAllowedEncodings() {


Incidentally, getEncodingId should be removed. Possibly replaced by a URN, but that can live in the CoderEncoder. And getAllowedEncodings fails to address the "version N+2" problem effectively, so I think that design is DOA.

kennknowles · 2017-04-25T03:09:22Z

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

 * @param <V> the type of the values of the KVs being transcoded
 */
-public class MapCoder<K, V> extends StandardCoder<Map<K, V>> {
+public class MapCoder<K, V> extends CustomCoder<Map<K, V>> {


I don't see the value in making this a CustomCoder. Why?

kennknowles · 2017-04-25T03:09:58Z

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

 * @param <T> the type of the values being transcoded
 */
-public class NullableCoder<T> extends StandardCoder<T> {
+public class NullableCoder<T> extends CustomCoder<T> {


Ditto, here. Even though null is a Java-specific interpretation, it is universal to have a coder that is "either a thing or nothing".

lukecwik · 2017-04-25T15:37:42Z

The reasoning to swap everything to be a custom coder is to remove the need to create URNs and specifications for all the non-Fn API coders. Swapping to using customcoder is one way of freeing us up in the future to define a specification equivalent to what it encodes and provide an URN.

kennknowles · 2017-04-25T17:10:17Z

OK, that sounds fine as a temporary measure. Obviously many of these coders already exist as a well-defined language-independent format. But it is fine to not commit to it right now.

kennknowles · 2017-04-25T17:10:27Z

(We can also always just bump the URN version)

tgroh · 2017-04-25T17:10:36Z

There is no restriction that a class that extends CustomCoder be a class that serializes purely via Java serialization. The mechanism to actually register custom urn-based serializers isn't implemented yet. Coders which extend CustomCoder and have no specifically defined Serialization mechanism (e.g. shared urn and payload format) will be serialized via Java, but the idea is that we'll be able to register additional coder serializations (e.g. NullableCoder, MapCoder, etc)

My understanding of the "meaning" of StandardCoder is that it's a coder that must be understood by the runner.

lukecwik · 2017-04-25T17:41:15Z

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

-   * @deprecated For {@code AvroCoder} internal use only.
   */
  // TODO: once we can remove this deprecated function, inline in constructor.
-  @Deprecated


Can we make this method package private and marked with @VisibleForTesting now since they were marked @deprecated for that reason

https://issues.apache.org/jira/browse/BEAM-2077 for followup. Re-deprecated in the meantime.

lukecwik · 2017-04-25T17:41:20Z

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

-   * @deprecated For {@code AvroCoder} internal use only.
   */
  // TODO: once we can remove this deprecated function, inline in constructor.
-  @Deprecated


Can we make this method package private and marked with @VisibleForTesting now since they were marked @deprecated for that reason

https://issues.apache.org/jira/browse/BEAM-2077 for followup. Re-deprecated in the meantime.

lukecwik · 2017-04-25T17:44:20Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/TestStream.java

-    }
-
-    @Override
-    public void verifyDeterministic() throws NonDeterministicException {


I think this method should still exist.

I don't believe this Coder should exist; rm'd

lukecwik · 2017-04-25T19:44:54Z

LGTM

lukecwik · 2017-04-25T22:25:25Z

LGTM again, rebase/merge pain

kennknowles · 2017-04-26T04:12:10Z

Another problem is that compatibility across e.g. Flink savepoints or Dataflow's pipeline update depends on the coder having the same representation and transparent components. This may make them less stable.

coveralls · 2017-04-26T09:31:45Z

Changes Unknown when pulling 880d800 on tgroh:fewer_standard_coders into ** on apache:master**.

coveralls · 2017-04-26T11:17:20Z

Changes Unknown when pulling 880d800 on tgroh:fewer_standard_coders into ** on apache:master**.

coveralls · 2017-04-26T22:28:22Z

Changes Unknown when pulling 880d800 on tgroh:fewer_standard_coders into ** on apache:master**.

tgroh force-pushed the fewer_standard_coders branch from 6fc50b2 to ca05834 Compare April 25, 2017 01:08

tgroh force-pushed the fewer_standard_coders branch from ca05834 to 032e1ef Compare April 25, 2017 01:21

kennknowles reviewed Apr 25, 2017

View reviewed changes

fixup! Make Most StandardCoders CustomCoders

a475f7d

lukecwik reviewed Apr 25, 2017

View reviewed changes

tgroh added 2 commits April 25, 2017 10:54

fixup! Make Most StandardCoders CustomCoders

b105b6d

fixup! Make Most StandardCoders CustomCoders

297fd8d

lukecwik approved these changes Apr 25, 2017

View reviewed changes

tgroh added 4 commits April 25, 2017 13:14

fixup! Make Most StandardCoders CustomCoders

ef1a968

fixup! fixup! Make Most StandardCoders CustomCoders

cc0149c

fixup! fixup! fixup! Make Most StandardCoders CustomCoders

92a0966

fixup! Make Most StandardCoders CustomCoders

880d800

tgroh force-pushed the fewer_standard_coders branch from 13d9b79 to 880d800 Compare April 25, 2017 21:59

asfgit closed this in 0d69611 Apr 25, 2017

Conversation

tgroh commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgroh commented Apr 25, 2017

Uh oh!

kennknowles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Apr 25, 2017

Uh oh!

kennknowles commented Apr 25, 2017

Uh oh!

kennknowles commented Apr 25, 2017

Uh oh!

tgroh commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Apr 25, 2017

Uh oh!

lukecwik commented Apr 25, 2017

Uh oh!

kennknowles commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 26, 2017

Uh oh!

coveralls commented Apr 26, 2017

Uh oh!

coveralls commented Apr 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgroh commented Apr 24, 2017 •

edited

Loading

kennknowles commented Apr 26, 2017 •

edited

Loading