Remove Avro-related code from Java SDK "core" module #27851

aromanenko-dev · 2023-08-04T15:13:20Z

This is the final step to remove Avro dependency from Java SDK "core" after it was deprecated in 2.46.0 release.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

aromanenko-dev · 2023-08-07T13:43:50Z

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)


I think it should not be a breaking change, should it?

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 :/

Right but I don't think Beam supports a hot upgrade.
I don't see a way how to avoid such change.

We should make a note about this (as well as the larger change) in CHANGES.md.

Serialization isn't guaranteed to be stable either--if we're switching coders should we switch to a custom (or Beam-schema-based) coder here?

It worries me a bit that there is no workaround. I suppose we could advise copying the old CountingSource into your own project and using that directly.

The serialization might not be stable when upgrading a pipeline between Java versions (or, technically, if this class ever gets changed or moved).

Ahh, right. I guess a use case for that - it should be a streaming pipeline that was restarted with different (upgraded) version of Beam. Do we commit (Beam) to support this?

I don't know if it's a solid commitment (e.g. this is itself a breaking change), but we really try hard where feasible.

I would recommend creating a custom coder which is binary compatible to the Avro based one. This is actually rather trivial, Avro does zigzag varint encoding and the bytes of both longs in CounterMark are just written after another without anything added.

Zigzag encoding is also trivial to implement, or using org.apache.beam.vendor.grpc.v1p54p0.com.google.protobuf.CodedOutputStream.encodeZigZag64 (and Protobuf will always remain a core dependency):

public static long encodeZigZag64(final long n) { return (n << 1) ^ (n >> 63); } public static long decodeZigZag64(final long n) { return (n >>> 1) ^ -(n & 1); }

And varint encoding / decoding is already implemented in org.apache.beam.sdk.util.VarInt.

@mosche I think you have a point but it's overkill in this case. I'd stick with SchemaCoder as Robert suggested.

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ConvertHelpers.java

github-actions · 2023-09-15T17:00:55Z

Test Results

  1 257 files +1 111   1 257 suites +1 111 2h 52m 43s ⏱️ + 2h 13m 31s
10 432 tests +9 098 10 362 ✔️ +9 061 68 💤 +35 2 ❌ +2
10 456 runs +9 120 10 386 ✔️ +9 083 68 💤 +35 2 ❌ +2

For more details on these failures, see this check.

Results for commit f5ee525a. ± Comparison against base commit cf0cf3b.

This pull request removes 9 and adds 9105 tests. Note that renamed tests count towards both.

id]
id`]
org.apache.beam.sdk.extensions.sql.CalciteCannotParseSimpleIdentifiersTest ‑ testFailsToParseAlias[`field
org.apache.beam.sdk.extensions.sql.CalciteCannotParseSimpleIdentifiersTest ‑ testFailsToParseAlias[field
org.apache.beam.sdk.extensions.sql.impl.LazyAggregateCombineFnTest$UdafImplTest ‑ subclassGetUdafImpl[aggregateFn: org.apache.beam.sdk.extensions.sql.impl.LazyAggregateCombineFnTest$Sum@ac9fa95]
org.apache.beam.sdk.extensions.sql.impl.LazyAggregateCombineFnTest$UdafImplTest ‑ subclassGetUdafImpl[aggregateFn: org.apache.beam.sdk.extensions.sql.impl.LazyAggregateCombineFnTest$SumChild@67172cd]
org.apache.beam.sdk.testutils.jvmverification.JvmVerification ‑ verifyCodeIsCompiledWithJava8
org.apache.beam.sdk.testutils.jvmverification.JvmVerification ‑ verifyRunningJVMVersionIs17
org.apache.beam.sdk.testutils.jvmverification.JvmVerification ‑ verifyTestCodeIsCompiledWithJava17

DefaultPackageTest ‑ defaultPackageInvoker
org.apache.beam.examples.DebuggingWordCountTest ‑ testDebuggingWordCount
org.apache.beam.examples.MinimalWordCountTest ‑ testMinimalWordCount
org.apache.beam.examples.WindowedWordCountIT ‑ testWindowedWordCountInBatchDynamicSharding
org.apache.beam.examples.WindowedWordCountIT ‑ testWindowedWordCountInBatchStaticSharding
org.apache.beam.examples.WindowedWordCountIT ‑ testWindowedWordCountInStreamingStaticSharding
org.apache.beam.examples.WordCountIT ‑ testE2EWordCount
org.apache.beam.examples.WordCountTest ‑ testCountWords
org.apache.beam.examples.WordCountTest ‑ testExtractWordsFn
org.apache.beam.examples.complete.AutoCompleteTest ‑ testAutoComplete[0]
…

♻️ This comment has been updated with latest results.

aromanenko-dev · 2023-09-18T09:04:19Z

retest this please

aromanenko-dev · 2023-09-18T09:05:20Z

@mosche Could you take a look, please? I think it's ready for review.

github-actions · 2023-09-18T09:07:37Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

aromanenko-dev · 2023-09-25T16:01:23Z

Run Java_Pulsar_IO_Direct PreCommit

mosche

A couple of comments @aromanenko-dev
Are you sure enough time has already passed to remove this from core, it hasn't been deprecated for too long?

mosche · 2023-09-26T06:43:44Z

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)


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 :/

mosche · 2023-09-26T06:57:23Z

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)


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

We need this class for tests where SerializableCoder is used implicitly and we need to have another default coder to test a correct coder precedence. So I use MockDefaultCoder for that.

mosche · 2023-09-26T06:58:53Z

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)


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.

I renamed it to MockDefaultCoder and it's used in other test as well, so let's keep it as a separate file

kennknowles · 2023-10-13T14:25:41Z

Regarding whether enough time has passed: I am OK with it. Users have an easy migration path. Thanks for following through with this very difficult change!

robertwb

Thanks for finishing up this huge effort!

aromanenko-dev · 2023-10-13T16:58:25Z

Run Java_Examples_Dataflow_Java11 PreCommit

aromanenko-dev · 2023-10-13T16:58:35Z

Run Java_Examples_Dataflow_Java17 PreCommit

aromanenko-dev · 2023-10-13T16:58:46Z

Run Java_GCP_IO_Direct PreCommit

aromanenko-dev · 2023-10-14T18:06:15Z

Run Java_Examples_Dataflow_Java11 PreCommit

aromanenko-dev · 2023-10-14T18:09:04Z

Seems like all tests passed, failed jobs looks not related.
Java PreCommit passed but status was not updated on this page.

@mosche @robertwb @kennknowles
Thanks for review. Please, let me know if you think about some other checks that we have to run before merge.

kennknowles · 2023-10-16T16:35:36Z

The precommit timed out and I can't quite tell but maybe locally try: :runners:google-cloud-dataflow-java:worker:test

I don't think this requires cloud.

kennknowles · 2023-10-16T16:35:42Z

run java precommit

aromanenko-dev · 2023-10-17T11:42:44Z

retest this please

aromanenko-dev · 2023-10-17T14:55:32Z

Run Java_GCP_IO_Direct PreCommit

aromanenko-dev · 2023-10-17T16:13:47Z

Run Java PostCommit

aromanenko-dev · 2023-10-18T07:51:19Z

Run Java PreCommit

aromanenko-dev · 2023-10-18T07:51:47Z

Run Java_Amazon-Web-Services2_IO_Direct PreCommit

aromanenko-dev · 2023-10-18T08:25:42Z

Run Java PostCommit

aromanenko-dev · 2023-10-18T11:39:08Z

@kennknowles

The precommit timed out and I can't quite tell but maybe locally try: :runners:google-cloud-dataflow-java:worker:test

Yes, it passed locally

aromanenko-dev · 2023-10-18T11:40:51Z

There are two failing PreCommit actions but I don't think they are related.

So, if there are no objections or any other comments, I'd merge this PR.

robertwb · 2023-10-18T15:47:54Z

I think you can go ahead and merge.

…

On Wed, Oct 18, 2023 at 4:41 AM Alexey Romanenko ***@***.***> wrote: There are two failing PreCommit actions but I don't think they are related. So, if there are no objections or any other comments, I'd merge this PR. — Reply to this email directly, view it on GitHub <#27851 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADWVAPMYEHVCIGZTX5VQ2LX76545AVCNFSM6AAAAAA3ELBRNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRYGI3DSNZSGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

kennknowles · 2023-10-19T15:39:43Z

🎉 🎉 🎉 🎉

aromanenko-dev marked this pull request as draft August 4, 2023 15:13

github-actions bot added the java label Aug 4, 2023

aromanenko-dev commented Aug 7, 2023

View reviewed changes

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ConvertHelpers.java Outdated Show resolved Hide resolved

aromanenko-dev force-pushed the 25252_no_avro_in_core branch from 49832e7 to 0b63672 Compare August 7, 2023 15:25

github-actions bot added io gcp labels Aug 7, 2023

aromanenko-dev force-pushed the 25252_no_avro_in_core branch from 0b63672 to a2e6321 Compare August 7, 2023 15:41

aromanenko-dev mentioned this pull request Aug 14, 2023

Refactor ConvertHelpers in "core" to not depend on Avro #27991

Merged

3 tasks

aromanenko-dev force-pushed the 25252_no_avro_in_core branch 2 times, most recently from 4a194de to 57f7e9e Compare September 12, 2023 13:26

github-actions bot added extensions clickhouse timeseries labels Sep 12, 2023

aromanenko-dev force-pushed the 25252_no_avro_in_core branch from 57f7e9e to d981472 Compare September 13, 2023 11:30

github-actions bot added kafka zetasketch thrift labels Sep 13, 2023

aromanenko-dev force-pushed the 25252_no_avro_in_core branch from d981472 to f5ee525 Compare September 15, 2023 16:50

aromanenko-dev marked this pull request as ready for review September 18, 2023 09:04

aromanenko-dev changed the title ~~[WIP] Remove Avro-related code from Java SDK "core" module~~ Remove Avro-related code from Java SDK "core" module Sep 18, 2023

aromanenko-dev requested a review from mosche September 18, 2023 09:05

aromanenko-dev force-pushed the 25252_no_avro_in_core branch from f5ee525 to 07ce6ce Compare September 22, 2023 15:40

aromanenko-dev force-pushed the 25252_no_avro_in_core branch from 07ce6ce to 46e75c7 Compare September 25, 2023 16:13

mosche reviewed Sep 26, 2023

View reviewed changes

aromanenko-dev force-pushed the 25252_no_avro_in_core branch 2 times, most recently from 451a504 to f22a161 Compare October 13, 2023 15:44

robertwb approved these changes Oct 13, 2023

View reviewed changes

aromanenko-dev force-pushed the 25252_no_avro_in_core branch from f22a161 to 43a1d7d Compare October 16, 2023 13:19

aromanenko-dev force-pushed the 25252_no_avro_in_core branch from 43a1d7d to 9d175ec Compare October 17, 2023 11:44

Remove Avro-related code from Java SDK "core" module

0850a6c

aromanenko-dev force-pushed the 25252_no_avro_in_core branch from 9d175ec to 0850a6c Compare October 17, 2023 16:46

aromanenko-dev merged commit 5fd1245 into apache:master Oct 18, 2023
33 of 35 checks passed

aromanenko-dev added this to the 2.52.0 Release milestone Oct 18, 2023

This was referenced Nov 11, 2023

Relocate Avro dependencies in Fn Harness JAR #29407

Closed

Mark Avro as provided in harness JAR #29412

Merged

[Bug]: Can not use Avro over 1.8.2 with Beam 2.52.0 #29413

Closed

Remove Avro-related code from Java SDK "core" module #27851

Remove Avro-related code from Java SDK "core" module #27851

Conversation

aromanenko-dev commented Aug 4, 2023 • edited Loading

GitHub Actions Tests Status (on master branch)

aromanenko-dev Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 15, 2023 • edited Loading

Test Results

aromanenko-dev commented Sep 18, 2023

aromanenko-dev commented Sep 18, 2023 • edited Loading

github-actions bot commented Sep 18, 2023

aromanenko-dev commented Sep 25, 2023

mosche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aromanenko-dev Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennknowles commented Oct 13, 2023

robertwb left a comment

Choose a reason for hiding this comment

aromanenko-dev commented Oct 13, 2023

aromanenko-dev commented Oct 13, 2023

aromanenko-dev commented Oct 13, 2023

aromanenko-dev commented Oct 14, 2023

aromanenko-dev commented Oct 14, 2023 • edited Loading

kennknowles commented Oct 16, 2023

kennknowles commented Oct 16, 2023

aromanenko-dev commented Oct 17, 2023

aromanenko-dev commented Oct 17, 2023

aromanenko-dev commented Oct 17, 2023

aromanenko-dev commented Oct 18, 2023

aromanenko-dev commented Oct 18, 2023

aromanenko-dev commented Oct 18, 2023

aromanenko-dev commented Oct 18, 2023

aromanenko-dev commented Oct 18, 2023

robertwb commented Oct 18, 2023 via email

kennknowles commented Oct 19, 2023

aromanenko-dev commented Aug 4, 2023 •

edited

Loading

aromanenko-dev Aug 7, 2023 •

edited

Loading

github-actions bot commented Sep 15, 2023 •

edited

Loading

aromanenko-dev commented Sep 18, 2023 •

edited

Loading

aromanenko-dev Sep 26, 2023 •

edited

Loading

aromanenko-dev commented Oct 14, 2023 •

edited

Loading