[GitHub] [beam] RustedBones commented on a change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9

[GitBox <gi...@apache.org>]

RustedBones commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r771257284



##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java
##########
@@ -272,26 +263,8 @@ public Schema get() {
     }
   }
 
-  /**
-   * A {@link Serializable} object that lazily supplies a {@link ReflectData} built from the
-   * appropriate {@link ClassLoader} for the type encoded by this {@link AvroCoder}.
-   */
-  private static class SerializableReflectDataSupplier
-      implements Serializable, Supplier<ReflectData> {
-
-    private final Class<?> clazz;
-
-    private SerializableReflectDataSupplier(Class<?> clazz) {
-      this.clazz = clazz;
-    }
-
-    @Override
-    public ReflectData get() {
-      ReflectData reflectData = new ReflectData(clazz.getClassLoader());
-      reflectData.addLogicalTypeConversion(new JodaTimestampConversion());
-      return reflectData;
-    }
-  }
+  private final AvroSource.DatumReaderFactory<T> readerFactory;
+  private final AvroSink.DatumWriterFactory<T> writerFactory;

Review comment:
       Ideally I would have liked to have those as constructor parameter.
   As explained in the PR comment, this cases problem with the [`AvroCoderCloudObjectTranslator`](https://github.com/apache/beam/blob/master/runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/AvroCoderCloudObjectTranslator.java) which serializes the `AvroCoder` in a CloudObject (json). I don't know how to serialize the factories in that case 

##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java
##########
@@ -1338,7 +1383,7 @@ public void populateDisplayData(DisplayData.Builder builder) {
       abstract Builder<UserT, DestinationT, OutputT> setShardTemplate(
           @Nullable String shardTemplate);
 
-      abstract Builder<UserT, DestinationT, OutputT> setGenericRecords(boolean genericRecords);
+      abstract Builder<UserT, DestinationT, OutputT> setRecordClass(Class<OutputT> recordClass);

Review comment:
       This is a breaking change. It aligns better with the `Source` implementation.

##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java
##########
@@ -542,18 +550,21 @@
    * <p>If the output type is {@link GenericRecord} use {@link #writeCustomTypeToGenericRecords()}
    * instead.
    */
-  public static <UserT, OutputT> TypedWrite<UserT, Void, OutputT> writeCustomType() {
-    return AvroIO.<UserT, OutputT>defaultWriteBuilder().setGenericRecords(false).build();
+  public static <UserT, OutputT> TypedWrite<UserT, Void, OutputT> writeCustomType(
+      Class<OutputT> recordClass) {

Review comment:
       This is a breaking change. The class needs to be propagated for proper factories instanciation.




-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org