Curious why these failing tests didn't block submission. For now rollback seems to be the simplest option. However is there a path forward on Java 11, or is our model irretrievably broken on Java 11?
On Fri, Jul 21, 2023 at 8:57 AM Kenneth Knowles <k...@apache.org> wrote: > This is a tricky situation that I don't know how to resolve best. Here are > some pieces of information I know: > > 1. The reason we put certain generated classes in the same package is > because of Java's package-private access restriction. If they are in > another package the generated wrapper won't be able to invoke the needed > functions. I know this applies to a generated DoFnInvoker. I don't know if > it applies here. > > 2. The current status for Beam is that Beam itself is only > expected/required to be able to build with Java 8 and/or produce Java 8 > compatible bytecode, but users should be able to use it with their own Java > 11 or Java 17 code. This makes the testing scenario a bit tricky. We do > have tests that model this scenario but they did not catch this I guess. > > On Mon, Jul 17, 2023 at 1:19 AM Damon Douglas <damondoug...@apache.org> > wrote: > >> Good day, everyone, >> >> For clarity, I organize the following into situation, background, >> assessment, and proposal. >> >> Best, >> >> Damon >> >> ----------------------------------------- >> >> Situation >> >> Issue #26981 reports an IllegalArgumentException associated with the >> ByteBuddy dependency throwing the message "<some class> must be defined in >> the same package as <some other class>"[1]. I personally discovered this >> error blocking my own Schema-related tests. >> >> Background >> >> *1. PR #25578 introduced the error* >> >> As Issue #26981 reports[1], the error seems to be introduced with 2.48. >> Comparing v2.47.0 and v2.48.0[2] reveals that PR #25578 may have introduced >> this breaking change[3]. Said PR replaced ByteBuddy's >> ClassLoadingStrategy.Default.INJECTION[4] with getClassLoadingStrategy[5]. >> >> *2. Reverting PR #25578 resolves the error* >> >> To test this hypothesis, I cloned 41e6628 and ran: >> >> ./gradlew :sdks:java:core:check >> >> revealing several failing tests (see *Failing :sdks:java:core:check at >> 41e6628* below), some of which contained the >> familiar IllegalArgumentException "<some class> must be defined in the same >> package as <some other class>" message. >> >> After reverting changes found in #25578, the failing tests and the >> IllegalArgumentException were resolved. >> >> *3. Code related to PR #25578 has a back and forth history* >> >> There seems to be a back and forth removal and replacement history[6] >> between ByteBuddy's ClassLoadingStrategy.Default.INJECTION >> and getClassLoadingStrategy most recently PR #25578. Said PR's motivation >> is to prepare Beam for Java 17 compatibility, which explains the >> re-introduction of the breaking changes. >> >> *4. PR #25578 GitHub Actions checks pass* >> >> Examining the GitHub actions run reveals that PR #25578 checks passed[7]. >> However, examining the setup[8] more closely reveals that Java tests are >> executed using Java Version 8. The same is true in the latest 41e6628 >> commit[9]. To test whether the version of Java drives Issue #26981's >> error, I submitted a draft PR[10] with the version of Java set to 11 and >> found that the same errors resulted[11] as I found on my machine using the >> same Java version. >> >> Assessment >> >> My main impression is that: >> >> 1. checks did not reveal PR #25578's breaking changes[7] because the >> environment[8] used Java 8 instead of 11 >> 2. the back and forth removal and addition of PR #25578's changes >> does not solve current and future Java version compatibilities >> >> Proposal >> >> May we consider: >> >> 1. If not already planned, set >> .github/actions/setup-self-hosted-action/action.yml's Java version[12] to >> 11. >> 2. arriving at a consensus regarding PR #25578's breaking changes and >> what we need to do today and in the future; I don't have anything >> practical >> to propose or recommend >> >> References >> >> 1. https://github.com/apache/beam/issues/26981 >> 2. https://github.com/apache/beam/compare/v2.47.0...v2.48.0 >> 3. https://github.com/apache/beam/pull/25578 >> 4. >> >> https://javadoc.io/static/net.bytebuddy/byte-buddy/1.12.23/net/bytebuddy/dynamic/loading/ClassLoadingStrategy.Default.html#INJECTION >> 5. >> >> https://github.com/apache/beam/blob/68e19a596a5d0136ba4592be01888f487463c2f3/sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java#L32 >> 6. >> >> https://github.com/apache/beam/commits/68e19a596a5d0136ba4592be01888f487463c2f3/sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java >> 7. >> https://github.com/apache/beam/actions/runs/4759753495/jobs/8459342173 >> 8. >> >> https://github.com/apache/beam/actions/runs/4759753495/jobs/8459342173#step:3:28 >> 9. >> >> https://github.com/apache/beam/actions/runs/5558349809/jobs/10153336615#step:3:16 >> 10. >> >> https://github.com/apache/beam/actions/runs/5571881403/jobs/10177348958?pr=27515#step:3:16 >> 11. >> >> https://github.com/apache/beam/actions/runs/5571881403/jobs/10177348958?pr=27515#step:5:123 >> 12. >> >> https://github.com/apache/beam/blob/561870be80cc8696a926d7322b0f47ddeddcc222/.github/actions/setup-self-hosted-action/action.yml#L56 >> >> >> Failing :sdks:java:core:check at 41e6628 >> >> *Ran locally on*: >> >> openjdk 11.0.14.1 2022-02-08 >> OpenJDK Runtime Environment JBR-11.0.14.1.1-2043.11-jcef (build >> 11.0.14.1+1-b2043.11) >> OpenJDK 64-Bit Server VM JBR-11.0.14.1.1-2043.11-jcef (build >> 11.0.14.1+1-b2043.11, mixed mode) >> >> org.apache.beam.sdk.schemas.AvroSchemaTest > testPojoRecordToRow FAILED >> java.lang.IllegalArgumentException at AvroSchemaTest.java:451 >> >> org.apache.beam.sdk.schemas.AvroSchemaTest > testSpecificRecordToRow >> FAILED >> java.lang.IllegalArgumentException at AvroSchemaTest.java:386 >> >> org.apache.beam.sdk.schemas.JavaBeanSchemaTest > testMapFieldSetters >> FAILED >> java.lang.IllegalArgumentException at JavaBeanSchemaTest.java:460 >> >> org.apache.beam.sdk.schemas.JavaBeanSchemaTest > >> testRecursiveArraySetters FAILED >> java.lang.IllegalArgumentException at JavaBeanSchemaTest.java:385 >> >> org.apache.beam.sdk.schemas.JavaBeanSchemaTest > testMapFieldGetters >> FAILED >> java.lang.IllegalArgumentException at JavaBeanSchemaTest.java:437 >> >> org.apache.beam.sdk.schemas.JavaBeanSchemaTest > >> testRecursiveArrayGetters FAILED >> java.lang.IllegalArgumentException at JavaBeanSchemaTest.java:369 >> >> org.apache.beam.sdk.schemas.JavaFieldSchemaTest > testNestedArraysToRow >> FAILED >> java.lang.IllegalArgumentException at JavaFieldSchemaTest.java:602 >> >> org.apache.beam.sdk.schemas.JavaFieldSchemaTest > testEnumFieldToRow >> FAILED >> java.lang.IllegalArgumentException at JavaFieldSchemaTest.java:658 >> >> org.apache.beam.sdk.schemas.JavaFieldSchemaTest > >> testRecursiveArraySetters FAILED >> java.lang.IllegalArgumentException at JavaFieldSchemaTest.java:411 >> >> org.apache.beam.sdk.schemas.JavaFieldSchemaTest > testMapFieldGetters >> FAILED >> java.lang.IllegalArgumentException at JavaFieldSchemaTest.java:463 >> >> org.apache.beam.sdk.schemas.JavaFieldSchemaTest > >> testRecursiveArrayGetters FAILED >> java.lang.IllegalArgumentException at JavaFieldSchemaTest.java:394 >> >> org.apache.beam.sdk.schemas.JavaFieldSchemaTest > testNestedArraysFromRow >> FAILED >> java.lang.IllegalArgumentException at JavaFieldSchemaTest.java:580 >> >> org.apache.beam.sdk.schemas.utils.JsonUtilsTest > >> testGetRowToJsonBytesFunction FAILED >> java.lang.IllegalArgumentException at JsonUtilsTest.java:92 >> >> org.apache.beam.sdk.schemas.utils.JsonUtilsTest > >> testGetJsonStringToRowFunction FAILED >> java.lang.IllegalArgumentException at JsonUtilsTest.java:92 >> >> org.apache.beam.sdk.schemas.utils.JsonUtilsTest > >> testGetRowToJsonStringsFunction FAILED >> java.lang.IllegalArgumentException at JsonUtilsTest.java:92 >> >> org.apache.beam.sdk.schemas.utils.JsonUtilsTest > >> testGetJsonBytesToRowFunction FAILED >> java.lang.IllegalArgumentException at JsonUtilsTest.java:92 >> >
