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 > > >
