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
