Ismaël, I was looking into BEAM-5723, is it possible to relocate both guava and Cassandra client instead of not relocating Guava in BEAM-6620?
On Tue, Mar 5, 2019 at 11:23 PM Gleb Kanterov <g...@spotify.com> wrote: > I agree with the points that Kenneth has raised, mainly: > > > In both of the above approaches, diamond dependency problems between IOs > are quite possible. > > Option 1. With having more IO-s in Beam we would start hitting diamond > dependency problem more often. Relocating dependencies will help, but for > this, we should avoid exposing relocated classes to end-users of IO-s. I > can't speak about everything, but in the case of BigtableIO, it only > exposes proto-classes that aren't part of bigtable-client-core, and > shouldn't be relocated. > > Option 2. Without relocation, every other IO can be potentially broken, > and we can solve this problem on a case-by-case basis. In maven > world situation becomes a little better with requireUpperBoundDeps [1] from > maven-enforcer-plugin. I don't know if there is a similar solution for > gradle. > > Option 3. There is a potential future solution for dependency conflicts > between IO-s with Java 9 JPMS [2], however, it could take a while before we > could use it due to compatibility issues. > > As a short term solution, option 2 seems the best, we could go through > known conflicts and see if it's possible to resolve them, potentially > looking into option 1 that would take much more time. > > [1]: > https://maven.apache.org/enforcer/enforcer-rules/requireUpperBoundDeps.html > [2]: https://en.wikipedia.org/wiki/Java_Platform_Module_System > > > On Mon, Mar 4, 2019 at 4:45 PM Ismaël Mejía <ieme...@gmail.com> wrote: > >> That looks interesting but I am not sure if I understand correctly, >> isn't the problem that the system API (Bigtable, Cassandra, etc) >> exposes guava related stuff? Or in other words, wouldn't the >> transitivie version of guava leak anyway? >> If it does not I am pretty interested on doing this to fix the >> Cassandra IO from leaking too. >> https://issues.apache.org/jira/browse/BEAM-5723 >> >> On Thu, Feb 28, 2019 at 5:17 PM Kenneth Knowles <k...@apache.org> wrote: >> > >> > If someone is using BigTableIO with bigtable-client-core then having >> BigTableIO and bigtable-client-core both depend on Guava 26.0 is fine, >> right? Specifically, a user of BigTableIO after >> https://github.com/apache/beam/pull/7957 will still have non-vendored >> Guava on the classpath due to the transitive deps of bigtable-client-core. >> > >> > In any case it seems very wrong for the Beam root project to manage the >> version of Guava in BigTableIO since the whole point is to be compatible >> with bigtable-client-core. Would it work to delete our pinned Guava version >> [1] and chase down all the places it breaks, moving Guava dependencies >> local to places where an IO or extension must use it for interop? Then you >> don't need adapters. >> > >> > In both of the above approaches, diamond dependency problems between >> IOs are quite possible. >> > >> > I don't know if we can do better. For example, producing a >> bigtable-client-core where we have relocated Guava internally and using >> that could really be an interop nightmare as things that look like the same >> type would not be. Less likely to be broken would be bigtable-client-core >> entirely relocated and vendored, but generally IO connectors exchange >> objects with users and the users would have to use the relocated versions, >> so that's gross. >> > >> > Kenn >> > >> > [1] >> https://github.com/apache/beam/blob/master/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy#L353 >> > >> > >> > On Thu, Feb 28, 2019 at 2:29 AM Gleb Kanterov <g...@spotify.com> wrote: >> >> >> >> For the past week, two independent people have asked me if I can help >> with guava NoSuchMethodError in BigtableIO. It turns out we still have a >> potential problem with dependencies that don't vendor guava, in this case, >> it was bigtable-client-core that depends on guava-26.0. However, the root >> cause of the classpath problem was in the usage of a deprecated method from >> non-vendored guava in BigtableServiceClientImpl in the code path where we >> integrate with bigtable client. >> >> >> >> I created apache/beam#7957 [1] to fix that. There few other IO-s where >> we use non-vendored guava that we can fix using adapters. >> >> >> >> And there is an unknown number of conflicts between guava versions in >> our dependencies that don't vendor it, that as I understand it, could be >> fixed by relocating them, in a similar way we do for Calcite [2]. >> >> >> >> [1]: https://github.com/apache/beam/pull/7957 >> >> [2]: >> https://github.com/apache/beam/blob/61de62ecbe8658de866280a8976030a0cb877041/sdks/java/extensions/sql/build.gradle#L30-L39 >> >> >> >> Gleb >> >> >> >> On Sun, Jan 20, 2019 at 11:43 AM Gleb Kanterov <g...@spotify.com> >> wrote: >> >>> >> >>> I didn't look deep into it, but it seems we can put >> .idea/codeInsightSettings.xml into our repository where we blacklist >> packages from auto-import. See an example in >> JetBrains/kotlin/.idea/codeInsightSettings.xml. >> >>> >> >>> On Sat, Jan 19, 2019 at 8:03 PM Reuven Lax <re...@google.com> wrote: >> >>>> >> >>>> Bad IDEs automatically generate the wrong import. I think we need to >> automatically prevent this, otherwise the bad imports will inevitably slip >> back in. >> >>>> >> >>>> Reuven >> >>>> >> >>>> On Tue, Jan 15, 2019 at 2:54 AM Łukasz Gajowy < >> lukasz.gaj...@gmail.com> wrote: >> >>>>> >> >>>>> Great news. Thanks all for this work! >> >>>>> >> >>>>> +1 to enforcing this on dependency level as Kenn suggested. >> >>>>> >> >>>>> Łukasz >> >>>>> >> >>>>> wt., 15 sty 2019 o 01:18 Kenneth Knowles <k...@apache.org> >> napisał(a): >> >>>>>> >> >>>>>> We can enforce at the dependency level, since it is a compile >> error. I think some IDEs and build tools may allow the compile-time >> classpath to get polluted by transitive runtime deps, so protecting against >> bad imports is also a good idea. >> >>>>>> >> >>>>>> Kenn >> >>>>>> >> >>>>>> On Mon, Jan 14, 2019 at 8:42 AM Ismaël Mejía <ieme...@gmail.com> >> wrote: >> >>>>>>> >> >>>>>>> Not yet, we need to add that too, there are still some tasks to be >> >>>>>>> done like improve the contribution guide with this info, and >> document >> >>>>>>> how to generate a src build artifact locally since I doubt we can >> >>>>>>> publish that into Apache for copyright reasons. >> >>>>>>> I will message in the future for awareness for awareness when >> most of >> >>>>>>> the pending tasks are finished. >> >>>>>>> >> >>>>>>> >> >>>>>>> On Mon, Jan 14, 2019 at 3:51 PM Maximilian Michels < >> m...@apache.org> wrote: >> >>>>>>> > >> >>>>>>> > Thanks for the heads up, Ismaël! Great to see the vendored >> Guava version is used >> >>>>>>> > everywhere now. >> >>>>>>> > >> >>>>>>> > Do we already have a Checkstyle rule that prevents people from >> using the >> >>>>>>> > unvendored Guava? If not, such a rule could be useful because >> it's almost >> >>>>>>> > inevitable that the unvedored Guava will slip back in. >> >>>>>>> > >> >>>>>>> > Cheers, >> >>>>>>> > Max >> >>>>>>> > >> >>>>>>> > On 14.01.19 05:55, Ismaël Mejía wrote: >> >>>>>>> > > We merged today the PR [1] that changes most of the code to >> use our >> >>>>>>> > > new guava vendored dependency. In practice it means that most >> of the >> >>>>>>> > > imports of the classes were changed from `com.google.common.` >> to >> >>>>>>> > > `org.apache.beam.vendor.guava.v20_0.com.google.common.` >> >>>>>>> > > >> >>>>>>> > > This is a great improvement to fix a long existing problem of >> guava >> >>>>>>> > > leaking through some Beam modules. This also reduces the size >> of most >> >>>>>>> > > jars in the project because they don't need to relocate and >> include >> >>>>>>> > > guava anymore, they just use the vendored dependency. >> >>>>>>> > > >> >>>>>>> > > Kudos to Kenn Knowles, Lukasz Cwik, Scott Wegner and the >> others that >> >>>>>>> > > worked (are working) to make this possible. >> >>>>>>> > > >> >>>>>>> > > Sadly as a side effect of the merge of this PR multiple PRs >> were >> >>>>>>> > > broken so please review if yours was and do a rebase and fix >> the >> >>>>>>> > > imports to use the new vendored dependency. Sorry for the >> >>>>>>> > > inconvenience. From now one all uses of guava should use the >> vendored >> >>>>>>> > > version. Expect some updates in the docs. >> >>>>>>> > > >> >>>>>>> > > [1] https://github.com/apache/beam/pull/6809 >> >>>>>>> > > >> >>> >> >>> >> >>> >> >>> -- >> >>> Cheers, >> >>> Gleb >> >> >> >> >> >> >> >> -- >> >> Cheers, >> >> Gleb >> > > > -- > Cheers, > Gleb > -- Cheers, Gleb
