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 <[email protected]> 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 <https://github.com/apache/beam/pull/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 <[email protected]> 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 >> <https://github.com/JetBrains/kotlin/blob/master/.idea/codeInsightSettings.xml> >> . >> >> On Sat, Jan 19, 2019 at 8:03 PM Reuven Lax <[email protected]> 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 <[email protected]> >>> 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 <[email protected]> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>>> 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 >
