Hello Luke, Thank you for the detailed response. Based on the above, it sounds like the overhead required to resolve java dependency issues caused by exposing guava types in the api surface would be more of a downside than having a slightly awkward list of lists to represent a multimap in the proto api. I've modified this IO to use the proto types instead of the wrapper types from the client library in the user-facing api surface.
Thanks again, Daniel On Fri, Jun 26, 2020 at 5:13 PM Luke Cwik <lc...@google.com> wrote: > There have been many versions of Guava that have been released with some > methods that have been removed/modified and many methods that were marked > @Beta. Guava maintainers said from release 23? that they would no longer > make backwards incompatible changes to non beta APIs and released a tool to > help users ensure they aren't using beta APIs [1]. This tool is great but > there are already 1000s of released artifacts that were produced before the > API lockdown and that beta checker was added. > > There are a lot of libraries that are still on Guava 19 or older and users > still run into linkage errors[2]. Users regularly run a linkage checker to > help them diagnose diamond dependency issues to get a set of libraries that > work with Beam and their dependencies. For example, the Beam community saw > this issue[3] when Guava decided to overload popular Preconditions[4] > methods which lead to NoSuchMethodErrors. A google search for guava and > NoSuchMethodError/ClassNotFoundException shows many users who have hit > issues with Guava. > > Google has worked on helping users via a linkage checking tool[5] and > Beam[6] would like to be in a better place with dependencies by using BOMs > for dependency management but both of these efforts are still relatively > new. > > On Thu, Jun 25, 2020 at 9:28 AM Daniel Collins <dpcoll...@google.com> > wrote: > >> Hello all, >> >> In https://github.com/apache/beam/pull/11919, there is a concern about >> the fact that the underlying client library's Message class uses guava's >> ListMultimap to represent a String -> Collection<ByteString> mapping, and >> this type is exposed by the interface. A couple of questions: >> >> 1) I'd like to use the same primitive value types as the underlying >> client library for consistency. Given that both the client library and beam >> io have the same maintainers, is this likely to cause long term issues? I'd >> really like to avoid wrapping the client library types in an >> almost-identical-but-not-quite wrapper just to remove guava, and I don't >> think removing guava from the underlying type is reasonable. >> > > For designing methods within Beam, it is preferable that you don't make > guava or the underlying client library part of the API surface. If there > are simple substitutes for types that is great but for configuration > objects such as protos it is a judgement call. Most IOs don't use the > underlying configuration object but instead choose to expose certain > properties on a case by case basis since properties may not make sense to > expose. > > For return types within the third party lib, casting the type to the > closest type within Java and only using methods that are part of Java core > will reduce the risk that you will invoke a method that has been > changed/removed. For example > ImmutableList<String> getFoos(); // method on object you want to interact > with > ImmutableList<String> foo = getFoos(); // bad, you can invoke a method > that has been removed/modified. > List<String> foo = getFoos(); // good, doesn't depend on Guava exposed > APIs > > For method arguments in the third party library you don't really have much > of a choice since you will have to call an API to construct the object and > you'll need to validate that you are not using @Beta apis. You can only > try to minimize the API surface you interact with. > > >> 2) What is the blast radius of guava incompatibility issues? There is a >> presubmit that blocks beam from using unshaded guava, presumably because it >> has caused issues in the past. If I expose this type, and someone tries to >> use it with an incompatible guava version, will they fail if they try to >> use beam or try to use this IO specifically? >> > > There are two issues: > 1) Users need to solve the diamond dependency problem and provide a > version of Guava that works with Beam and their dependencies. Guava usage > within Beam and its transitive dependency tree makes that more difficult > for users since some users rely on the linkage checker to help figure out a > compatible set of dependencies. This means that the linkage checker will > find faults that may have not existed otherwise. > 2) If the user selects a non-compatible version of guava, then it depends: > * any static links between classes will poison all referenced classes if > one of them does something that causes a linkage error (e.g. static block > that calls a method that can't be found or a class that doesn't exist) > * if the code is loaded dynamically via service loaders/reflection, then > the entire application can fail as soon as the object is on the classpath > > >> >> Thanks! Looking forward to some feedback. >> > > In the end, it is a judgement call vs maintenance, ease of use for users, > and user support. > > 1: https://github.com/google/guava-beta-checker > 2: https://jlbp.dev/glossary.html#linkage-error > <https://jlbp.dev/glossary> > 3: https://issues.apache.org/jira/browse/BEAM-1411 > 4: > https://github.com/google/guava/commit/892e323fca32945cdfb25395ca6e346dd0fffa5b#diff-fe7358934fa6eba23c2791eb40cec030 > 5: > https://github.com/GoogleCloudPlatform/cloud-opensource-java/wiki/Linkage-Checker-Enforcer-Rule > 6: https://issues.apache.org/jira/browse/BEAM-9444 >
