mosche commented on code in PR #27851:
URL: https://github.com/apache/beam/pull/27851#discussion_r1336697367


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/CountingSource.java:
##########
@@ -485,8 +486,8 @@ public long getSplitBacklogBytes() {
    * The checkpoint for an unbounded {@link CountingSource} is simply the last 
value produced. The
    * associated source object encapsulates the information needed to produce 
the next value.
    */
-  @DefaultCoder(AvroCoder.class)
-  public static class CounterMark implements UnboundedSource.CheckpointMark {
+  @DefaultCoder(SerializableCoder.class)

Review Comment:
   This is certainly a breaking change. Any checkpoint that was serialized and 
persisted with a previous version of Beam will fail on deserialization once 
upgrading Beam :/



##########
sdks/java/core/src/test/java/org/apache/beam/sdk/coders/DefaultCoderTest.java:
##########
@@ -39,7 +39,7 @@ public class DefaultCoderTest {
 
   @Rule public ExpectedException thrown = ExpectedException.none();
 
-  @DefaultCoder(AvroCoder.class)
+  @DefaultCoder(MockAvroCoder.class)

Review Comment:
   Please rename the record class and coder to not contain Avro. Also, I'd 
recommend to inline the coder impl into the test class to limit its scope. 



##########
sdks/java/core/src/test/java/org/apache/beam/sdk/coders/CoderRegistryTest.java:
##########
@@ -472,7 +472,7 @@ public void testCoderPrecedence() throws Exception {
     assertEquals(SerializableCoder.of(MyValueC.class), 
registry.getCoder(MyValueC.class));
   }
 
-  @DefaultCoder(AvroCoder.class)
+  @DefaultCoder(MockAvroCoder.class)

Review Comment:
   Just use SerializableCoder here if the intention is to just test precedence 
of coders, any mentioning of Avro will just confuse people.



-- 
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]

Reply via email to