kennknowles commented on code in PR #30235:
URL: https://github.com/apache/beam/pull/30235#discussion_r1486702071
##########
runners/direct-java/build.gradle:
##########
@@ -40,9 +40,6 @@ applyJavaNature(
include(project(path: it, configuration: "shadow"))
}
}
- relocate "org.apache.beam.runners.core",
getJavaRelocatedPath("runners.core")
Review Comment:
Yes it is a lot to put in a comment but let me know what you think should go
in the code;
See the comment on line 22. It is complicated and actually wrong...
- Java-to-proto translations can only be registered once or they will
cause a failure.
- The worry was: two runners could have registered different Java-to-proto
translations and they would conflict, so the direct runner would relocate all
of its registrations so the two runners could translate differently
- it doesn't actually work this way (even with relocation you can't
actually have two different translations really)
- it would not have caused a conflict because the registrations in
runners-core-construction would be the same AutoService class and not conflict
- proto translation really should not exist differently per runner, but
there are some hacks needed because of how we didn't put URNs in the core SDK,
so runners have "translators" that are just there to give a URN to a
non-serializable transform
Another annoying thing is that `org.apache.beam.runners.core` is
execution-time utilities, but `org.apache.beam.runners.core.construction` was
not. I think that partly this is accidentally catching it just because it has
to be a package prefix. Honestly we could have just called it `construction`
instead of `core.construction` and not had that problem.
Now the reason we _must_ get rid of it: after relocation, the registered
`AvroCoderTranslatorRegistrar` is a subclass or
`org.apache.beam.core.construction.CoderTranslatorRegistrar` but it is not a
subclass of
`org.apache.beam.runners.direct.relocated...org.apache.beam.core.construction.CoderTranslatorRegistrar`.
A possible alternative might have been to also include the avro extension
and relocation into the direct runner shadowClosure but I don't want to add a
dependency on avro.
Plus, this is temporary because after this PR it can all collapse into the
core SDK and there is no more core-construction.
##########
runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/CoderTranslationTest.java:
##########
@@ -152,14 +150,12 @@ public static Iterable<Coder<?>> data() {
StringUtf8Coder.of(),
SerializableCoder.of(Record.class),
new RecordCoder(),
- KvCoder.of(
Review Comment:
Yes it introduces a circular dependency. I split the test case into
`AvroCoderTranslationTest` already, no? I did not keep the KV since I don't
think we need to test that we can correctly serialize nested coders - that is
tested in lots of places and does not depend on what the nested coder is.
Is there a specific test case or cases you want me to add to
`AvroCoderTranslationTest`? I am happy to do it but I'm not sure the most
important scope (right now it just tests the coder from line 157).
##########
runners/direct-java/src/main/java/org/apache/beam/runners/direct/TransformEvaluatorRegistry.java:
##########
@@ -132,9 +131,6 @@ public static class DirectTransformsRegistrar implements
TransformPayloadTransla
.put(
DirectTestStream.class,
TransformPayloadTranslator.NotSerializable.forUrn(DIRECT_TEST_STREAM_URN))
- .put(
Review Comment:
That's right. Without the relocation from the other long comment, this one
and the one in the flink runner conflict. But they register the same thing, and
it doesn't do anything, and it is totally standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
