Thanks for the feedback, Ankit. Yes, we could skip creating the shaded guava artifact.
However, creating an explicit shaded guava for phoenix has the following advantages: 1. It saves space (we have one set of shaded guava classes, instead of three (one for tephra, omid, and phoenix each) 2. If we switch over all code to it, then it decouples the Guava version that we use in the phoenix code from the guava version used by Hadoop, solving the Hadoop 3.3 guava version problem, and possibly many future headaches. On Wed, Jun 10, 2020 at 1:38 AM Ankit Singhal <ankitsingha...@gmail.com> wrote: > Thanks, Istvan for putting up a detailed plan > > +1 for removing twill(as I can see it is recently moved to Attic) and > adding whatever convenience it's bringing in Omid/tephra project itself. > > Instead of creating a shaded artifact of Guava and then refer it in Omid > and Tephra, can't we just short-circuit your step 2 with 3 and use > relocation of the maven-shaded plugin to produce the shaded artifacts for > Tephra/Omid as we are anyways shading everything including public API? > > > > On Tue, Jun 2, 2020 at 8:25 AM Josh Elser <els...@apache.org> wrote: > > > Sounds like a well-thought-out plan to me. > > > > If we're going through and changing Guava, it may also be worthwhile to > > try to eliminate the use of Guava in our "public API". While the shaded > > guava eliminates classpath compatibility issues, Guava could (at any > > point) drop a class that we're using in our API and still break us. That > > could be a "later" thing. > > > > The only thing I think differently is that 4.x could (at some point) > > pick up the shaded guava artifact you describe and make the change. > > However, that's just for the future -- the call can be made if/when > > someone wants to do that :) > > > > On 6/2/20 10:01 AM, Istvan Toth wrote: > > > Hi! > > > > > > There are two related dependency issues that I believe should be solved > > in > > > Phoenix to keep it healthy and supportable. > > > > > > The Twill project has been officially terminated. Both Tephra and Omid > > > depend on it, and so transitively Phoenix does as well. > > > > > > Hadoop 3.3 has updated its Guava to 29, while Phoenix (master) is still > > on > > > 13. > > > None of Twill, Omid, Tephra, or Phoenix will run or compile against > > recent > > > Guava versions, which are pulled in by Hadoop 3.3. > > > > > > If we want to work with Hadoop 3.3, we either have to update all > > > dependencies to a recent Guava version, or we have to build our > artifacts > > > with shaded Guava. > > > Since Guava 13 has known vulnerabilities, including in the classpath > > causes > > > a barrier to adaptation. Some potential Phoenix users consider > including > > > dependencies with > > > known vulnerabilities a show-stopper, they do not care if the > > vulnerability > > > affects Phoenix or not. > > > > > > I propose that we take following steps to ensure compatibility with > > > upcoming Hadoop versions: > > > > > > *1. Remove the Twill dependency from Omid and Tephra* > > > It is generally not healthy to depend on abandoned projects, but the > fact > > > Twill also depends (heavily) on Guava 13, makes removal the best > > solution. > > > As far as I can see, Omid and Tephra mostly use the ZK client from > Twill, > > > as well as the (transitively included) Guava service model. > > > Refactoring to use the native ZK client, and to use the Guava service > > > classes directly shouldn't be too difficult. > > > > > > *2. Create a shaded guava artifact for Omid and Tephra* > > > Since Omid and Tephra needs to work with Hadoop2 and Hadoop3 (including > > the > > > upcoming Hadoop 3.3), which already pull in Guava, we need to use > > different > > > Guava internally. > > > (similar to the HBase-thirdparty solution, but we need a separate one). > > > This artifact could live under the Phoenix groupId, but we'll need to > be > > > careful with the circular dependencies. > > > > > > *3. Update the Omid and Tephra to use the shaded Guava artifact* > > > Apart from handling the mostly trivial, "let's break API compatibility > > for > > > the heck of it" Guava changes, the Guava Service API that both Omid and > > > Tephra build on has changed significantly. > > > This will mean changes in the public (Phoenix facing) APIs. All Guava > > > references will have to be replaced with the shaded guava classes from > > step > > > 2. > > > > > > *3. Define self-contained public APIs for Omid and Tephra* > > > To break the public API's dependency on Guava, redefine the public APIs > > in > > > such a way that they do not have Guava classes as ancestors. > > > This doesn't mean that we decouple the internal implementation from > > Guava, > > > simply defining a set of java Interfaces that matches the existing > > (updated > > > to recent Guava Service API) > > > interface's signature, but is self-contained under the Tephra/Omid > > > namespace should do the trick. > > > > > > *4. Update Phoenix to use new Omid/Tephra API* > > > i.e. use the new Interface that we defined in step 3. > > > > > > *5. Update Phoenix to work with Guava 13-29.* > > > We need to somehow get Phoenix work with both old and new Guava. > > > Probably the least disruptive way to do this is reduce the Guava use to > > the > > > common subset of 13.0 and 29.0, and replace/reimplement the parts that > > > cannot be resolved. > > > Alternatively, we could rebase to the same shaded guava-thirdparty > > library > > > that we use for Omid and Tephra. > > > > > > For *4.x*, since we cannot get rid of Guava 13 ever, *Step 5 *is not > > > necessary > > > > > > I am very interested in your opinion on the above plan. > > > Does anyone have any objections ? > > > Does anyone have a better solution ? > > > Is there some hidden pitfall that I hadn't considered (there certainly > > > is...) ? > > > > > > best regards > > > > > > Istvan > > > > > > -- *István Tóth* | Staff Software Engineer st...@cloudera.com <https://www.cloudera.com> [image: Cloudera] <https://www.cloudera.com/> [image: Cloudera on Twitter] <https://twitter.com/cloudera> [image: Cloudera on Facebook] <https://www.facebook.com/cloudera> [image: Cloudera on LinkedIn] <https://www.linkedin.com/company/cloudera> <https://www.cloudera.com/> ------------------------------
