Sorry, please ignore the previous empty reply … On 17.05.22, 09:31, "Moritz Mack" <mm...@talend.com> wrote:
On 16.05.22, 18:09, "Robert Bradshaw" <rober...@google.com> wrote: On Mon, May 16, 2022 at 8:53 AM Alexey Romanenko <aromanenko....@gmail.com> wrote: > >> On 13 May 2022, at 18:38, Robert Bradshaw <rober...@google.com> wrote: >> >> We should probably remove the experimental annotations from > SchemaCoder at this point. > > Is there anything that stops us from that? It's a commitment to not change things, but I think we're at that point by now. > Having that in mind, here’s my 2 cents on the options mentioned: > > Bump Avro in core: > > This will seamlessly work for users that just need a coder for complex types, > but don’t use Avro themselves. > Anyone using Avro APIs from user code might get into trouble. > > Support different Avro versions & making the dependency provided > > In terms of user impact, making Avro provided will be very troublesome. > Unless Avro remains available on the classpath through different ways, this > would break things in a really bad way. Worst of all, it might not even break > at compile time but at runtime only. > I don’t think this option requires Avro being a provided dependency. If the > default is kept as is, anyone can (safely) bump Avro to a higher (supported) > version on their classpath. In that case it would be good to warn users about > CVEs in logs if they remain on an old version of Avro. But I’m not sure how > feasible (reliable) it is to detect the Avro minor version given that things > might be repackaged into fat jars. > > This seems both difficult and potentially dangerous. > > What do you mean by “potentially dangerous”, Robert? Any specific point > mentioned above or just having another version of Avro jars in the classpath? The most dangerous is if a change in avro version had slightly [1] different semantics when it was swapped out. I think the chances of that are not too high, but it'll be hard to detect. Mostly I'm concerned about [2] breaking at runtime in the case of a missing or incompatible avro. [1] Don’t you think these can be mitigated by testing relevant parts of Beam against a number of supported Avro versions the same way parts of Beam are tested against various different Hadoop versions. [2] I agree, this is a huge concern and for that reason I believe Avro cannot be turned into a provided dependency. It would be a dangerous trap for users. But if the default Avro dependency remains as is and Beam is tested to work with a well-defined set of Avro versions (swapping out the default using standard dependency resolution rules), I’d consider the risk to be rather manageable. >> Extract Avro as an extension > I think a change that breaks loudly at compile time (if the features > in question are being depended on), with a clear fix (e.g. "depend on > this instead") is the "cleanest" way to do a breaking change. > > On this note, would it be possible to do explicit "without avro" and > "with avro" targets/variants in the core module, with plain old "core" > being an alias first to the latter, and then to the former, according > to some deprecation timeline? (I'm not familiar enough with > gradle/maven to know if this is possible.) This could unblock users > needing a new avro in the short run while providing a longer-term > migration path. (Ideally the (relatively limited) avro parts of core > would also get moved to their own extension under their own namespace, > and the core one would remain solely for backwards compatibility > reasons.) > > > Good point. Are you talking about keeping the default “core" artifacts with > old Avro version (as for now) as a main dependency and provide additionally > other “core” artifacts with a recent Avro version or even as provided for > users that need other Avro versions? I'm thinking about keeping the existing "core" artifact, and also shipping an [3] "avro-less" core target (whether that be via shading or total removal, but ripping it out of the public API). We would then ship a [4] separate "avro" target (TBD if providing the dependency or not makes the most sense here) that the existing core would depend on and would be a target for avro users to migrate to. [3] It wouldn’t be too hard modelling this using feature variants. Though, I’m worried about the complexity this creates. In the end, every single Beam module depends on core, which would then depend on the new Avro target. Effectively, we would need to build two different versions of each module depending on either core or the avro-less core. If there would be a well-planned, temporary transition period ahead, it should be fine. But I fear with Beam 3 not even being on the horizon, we’d have to carry this forward for far too long. [4] Also, what are your thoughts about the new Avro target? It could be done just by moving respective code into a new module (maintaining all packages / fully qualified names). Or “clean” by moving classes adequately but breaking fully qualified names / imports of users. Using the former approach obviously avoids users having to figure out the correct new imports. On the other hand, it might become a blocker supporting Java 11 as it conflicts with the new module system. > To finally make progress on this topic, I’d suggest starting off with > supporting multiple different Avro versions in core (2) but to keep the > default Avro version as is. As stated above, I think it's safer to make the avroless version a new target initially than change core. > At the same time, extracting Avro as extension in a backwards compatible way > seems to be the right path forward long term together with a well > communicated deprecation plan. > > > > Best, > > Moritz > > > > > > From: Brian Hulette <bhule...@google.com> > Date: Thursday, 12. May 2022 at 23:21 > To: dev <dev@beam.apache.org> > Subject: Re: [DISCUSS] Next steps for update of Avro dependency in Beam > > Regarding Option (3) "but keep and shade Avro for “core” needs as v.1.8.2 > (still have an issue with CVEs)" Do we actually need to keep avro in core for > any reason? I thought we only had it in core for AvroCoder, schema support, > and > > ZjQcmQRYFpfptBannerStart > > This Message Is From an External Sender > > This message came from outside your organization. > > Exercise caution when opening attachments or clicking any links. > > ZjQcmQRYFpfptBannerEnd > > Regarding Option (3) "but keep and shade Avro for “core” needs as v.1.8.2 > (still have an issue with CVEs)" > > > > Do we actually need to keep avro in core for any reason? I thought we only > had it in core for AvroCoder, schema support, and IOs, which I think are all > reasonable to separate out into an extension (this would be comparable to the > protobuf extension). To confirm I just grepped for files in core that import > avro: > > ❯ grep -liIrn 'import org\.apache\.avro' sdks/java/core/src/main > sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AvroByteBuddyUtils.java > sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ConvertHelpers.java > sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AvroUtils.java > sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/AvroRecordSchema.java > sdks/java/core/src/main/java/org/apache/beam/sdk/io/DynamicAvroDestinations.java > sdks/java/core/src/main/java/org/apache/beam/sdk/io/SerializableAvroCodecFactory.java > sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroSink.java > sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroSource.java > sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroSchemaIOProvider.java > sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java > sdks/java/core/src/main/java/org/apache/beam/sdk/io/ConstantAvroDestination.java > sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroGenericCoder.java > sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java > > Brian > > > > On Thu, May 12, 2022 at 2:08 PM Robert Bradshaw <rober...@google.com> wrote: > > Keeping avro in our public (core) API and as an internal dependency > seems to be a recurring pain point, I would be all for pulling it out > (basically option 3) and subsequently updating our internal version > (hopefully no backwards compatibility issues here) and letting the > extension live with a variety of versions (insofar as this is > feasible). > > On Thu, May 12, 2022 at 10:29 AM Alexey Romanenko > <aromanenko....@gmail.com> wrote: > > > Hi everyone, > > Sorry in advance for a long email. > TL;DR: Let’s discuss the next steps to update Avro dependency in Beam. > > I’d like to come back to this old and quite sensitive topic here which is > Apache Avro version update in Beam. Along the time, we already had several > discussions on this (for example [1]) but without any concrete resolutions in > the end, iirc. > > As we all know, Beam still depends on quite old Avro version 1.8.2 and there > were some attempts to bump it to more recent ones. One of the main reasons to > bump an Avro version, imho, is that Avro 1.8.2 dependency brings several CVEs > [2], but the latest Avro 1.11.0 brings only one [3] > > In the same time, this update with introduce some incompatible changes that > Avro has between versions and this may affect Beam users and potentially it > may affect transitive dependencies while using Beam with other project that > use Avro as well: > - Avro completely moved to java.time.* instead of org.joda.time.*. So, we > need to adjust date/time conversions from/to Beam schema accordingly since > Beam schema still uses joda.time. It will require users to regenerate already > generated Java code with avro-compiler (if any) otherwise it won’t compile; > - Some minor changes in Avro dependencies and user API; > - Something else? > > I know that here, on the list, we have people from Avro community that are > much more experienced in this than me - so, please correct me if I say > something wrong or not 100% correct. > > > In Beam, we also performed several attempts to update Avro - for example, > [4], [5], [6] and others. > > To make such update easier in the future, we also discussed to move Avro > dependency out of core Beam [7] and there were an attempt to do that [8] by > finally this PR was closed with a resolution that it’s not actually needed > and we may just want to test Beam with different Avro versions [9] > > The latest work on this was a PR to support several versions of Avro in Beam > (1.8.x and 1.9.x) [10] which still introduces some breaking changes for > users, iirc. > > So, seems that we are a bit stuck on this topic, though, imho, we need to > decide how move forward mostly because of CVEs in old Avro versions and > future Avro updates in Beam. > > The potential options (as I can see them): > > 1) Bump Avro dependency to the latest one (1.11.0) or the possible more > recent one > - Pros: > - latest/recent Avro dependency; > - potentially easy to update in the future; > - Cons: > - breaking change for users; > - potentially issues with other projects that use Avro (like Apache Spark > e.g.). > > 2) Support different Avro versions in Beam, make Avro dependency provided > - Pros: > - user decides which versions to use; > - easy to update in the future; > - Cons: > - breaking change for users; > - not fact that it’s possible to implement in reality; > - more tests to test Beam with different Avro versions > > 3) Extract Avro as an extension, like we do for other formats, and update to > latest Avro version, but keep and shade Avro for “core” needs as v.1.8.2 > (still have an issue with CVEs) > > 4) Anything else? > > > Please, share your thoughts on this and correct me if I stated something > wrong. The goal of this discussion is finally to move forward with Avro > update topic. > > — > Alexey > > > [1] > https://urldefense.com/v3/__https://lists.apache.org/thread/bkwrbqg2nwp1xq1j57xt3kvmy93vpj9r__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FafuMdp7Q$<https://urldefense.com/v3/__https:/lists.apache.org/thread/bkwrbqg2nwp1xq1j57xt3kvmy93vpj9r__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FafuMdp7Q$> > [2] > https://urldefense.com/v3/__https://mvnrepository.com/artifact/org.apache.avro/avro/1.8.2__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FYIRwfz1A$<https://urldefense.com/v3/__https:/mvnrepository.com/artifact/org.apache.avro/avro/1.8.2__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FYIRwfz1A$> > [3] > https://urldefense.com/v3/__https://mvnrepository.com/artifact/org.apache.avro/avro/1.11.0__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6Fa7FyrH-w$<https://urldefense.com/v3/__https:/mvnrepository.com/artifact/org.apache.avro/avro/1.11.0__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6Fa7FyrH-w$> > [4] > https://urldefense.com/v3/__https://github.com/apache/beam/pull/9779__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FYPzULSoA$<https://urldefense.com/v3/__https:/github.com/apache/beam/pull/9779__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FYPzULSoA$> > [5] > https://urldefense.com/v3/__https://github.com/apache/beam/pull/17372__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FZ1EJn6zw$<https://urldefense.com/v3/__https:/github.com/apache/beam/pull/17372__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FZ1EJn6zw$> > [6] > https://urldefense.com/v3/__https://github.com/apache/beam/pull/17246__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FbBlkHi_A$<https://urldefense.com/v3/__https:/github.com/apache/beam/pull/17246__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FbBlkHi_A$> > [7] > https://urldefense.com/v3/__https://lists.apache.org/thread/fw4w6xgm05nl5cg502co97pt6cygt4on__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FaisZP_Dg$<https://urldefense.com/v3/__https:/lists.apache.org/thread/fw4w6xgm05nl5cg502co97pt6cygt4on__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FaisZP_Dg$> > [8] > https://urldefense.com/v3/__https://github.com/apache/beam/pull/12748__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FZvPO_Cfw$<https://urldefense.com/v3/__https:/github.com/apache/beam/pull/12748__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FZvPO_Cfw$> > [9] > https://urldefense.com/v3/__https://lists.apache.org/thread/y76wjqprm8dyfxxfwcqbzxtht2qkrgzg__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FZHw_JT_g$<https://urldefense.com/v3/__https:/lists.apache.org/thread/y76wjqprm8dyfxxfwcqbzxtht2qkrgzg__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FZHw_JT_g$> > [10] > https://urldefense.com/v3/__https://github.com/apache/beam/pull/16271__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FZBoQ0uSQ$<https://urldefense.com/v3/__https:/github.com/apache/beam/pull/16271__;!!CiXD_PY!Uf7prSniW96ajGEcqZrwabMQ7X85axEPG2_VFpwk98-WAGoZ6NlRTLVx_1jeYk7PKEay6FZBoQ0uSQ$> > > > > > > > > As a recipient of an email from Talend, your contact personal data will be on > our systems. Please see our privacy notice. > > As a recipient of an email from Talend, your contact personal data will be on our systems. Please see our privacy notice. <https://www.talend.com/privacy/> As a recipient of an email from Talend, your contact personal data will be on our systems. Please see our privacy notice. <https://www.talend.com/privacy/>
