Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Won't Fix
    • Affects Version/s: 1.7.3
    • Fix Version/s: None
    • Component/s: build
    • Labels:
      None

      Description

      presto-accumulo connector embeds Accumulo client org.apache.accumulo:accumulo-core. Accumulo uses Guava. Since Guava APIs are not strictly backwards compatible, it's necessary to shade guava dependency or library users need to use same Guava version (i.e. cannot upgrade/downgrade at will).

      Please shade Guava dependency.

        Issue Links

          Activity

          Hide
          ctubbsii Christopher Tubbs added a comment -

          I think this would create more problems than it solves. Dependency management/dependency convergence is a hard problem, but it is currently a problem left to downstream. The class path is ultimately the responsibility for the users deploying the software. I do not think Accumulo can, or should, take on the responsibility to handle all the bugs and compatibility issues which could arise in any one of our dependencies, which we've bundled into our project. It would also cause problems for users who are already doing their own dependency convergence, because then they would have to de-bundle.

          I'm closing this issue for now, but my thoughts here are only my own. We have not discussed this much on the mailing lists. Feel free to open a dialogue at dev@accumulo.apache.org if you wish to discuss it further, as this would be a change with big implications, and would need to have some serious discussion/debate.

          In the meantime, please also feel free to continue to comment here, with suggestions for mitigations. If you have a patch to do the shading, or some other workaround for dependency convergence, posting here could be useful for other users in this circumstance, even if we don't reopen the issue and apply it upstream.

          Show
          ctubbsii Christopher Tubbs added a comment - I think this would create more problems than it solves. Dependency management/dependency convergence is a hard problem, but it is currently a problem left to downstream. The class path is ultimately the responsibility for the users deploying the software. I do not think Accumulo can, or should, take on the responsibility to handle all the bugs and compatibility issues which could arise in any one of our dependencies, which we've bundled into our project. It would also cause problems for users who are already doing their own dependency convergence, because then they would have to de-bundle. I'm closing this issue for now, but my thoughts here are only my own. We have not discussed this much on the mailing lists. Feel free to open a dialogue at dev@accumulo.apache.org if you wish to discuss it further, as this would be a change with big implications, and would need to have some serious discussion/debate. In the meantime, please also feel free to continue to comment here, with suggestions for mitigations. If you have a patch to do the shading, or some other workaround for dependency convergence, posting here could be useful for other users in this circumstance, even if we don't reopen the issue and apply it upstream.
          Hide
          vines John Vines added a comment -

          One of the things I've seen projects do is shade the dependency in by
          dynamically pulling in and converting the packages into something unique
          for the project. This negates any problems with compatibility aside from
          shaded projects leaking into the api

          On Fri, Sep 8, 2017, 7:26 PM Christopher Tubbs (JIRA) <jira@apache.org>

          Show
          vines John Vines added a comment - One of the things I've seen projects do is shade the dependency in by dynamically pulling in and converting the packages into something unique for the project. This negates any problems with compatibility aside from shaded projects leaking into the api On Fri, Sep 8, 2017, 7:26 PM Christopher Tubbs (JIRA) <jira@apache.org>
          Hide
          findepi Piotr Findeisen added a comment -

          Are Guava classes part of the Accumulo API?

          Show
          findepi Piotr Findeisen added a comment - Are Guava classes part of the Accumulo API?
          Hide
          ctubbsii Christopher Tubbs added a comment -

          No, Guava classes are not part of the API. Or, at least, they shouldn't be.

          Show
          ctubbsii Christopher Tubbs added a comment - No, Guava classes are not part of the API. Or, at least, they shouldn't be.
          Hide
          findepi Piotr Findeisen added a comment -

          Christopher Tubbs, that's good!

          Then I don't know what problems would shading of Guava introduce for projects using Accumulo. (And i think I know what problems will it solve)

          As it is now, I can't use Accumulo and Guava 22. This is because Accumulo uses Guava 14 and e.g HostAndPort class (marked @Beta in that Guava 14) and Guava decided to deprecate and later remove getHostText() method.

          According to Guava docs: https://github.com/google/guava/wiki/PhilosophyExplained#beta-apis , if you use Beta APIs and you are a library, you should shade (repackage) the Guava dependency. In fact, even if you do not use Beta APIs, then you should still upgrade quite regularly (https://github.com/google/guava/wiki/PhilosophyExplained#non-beta-apis). Since Guava 14 they had enough time to deprecate and remove non-Beta APIs too.

          Show
          findepi Piotr Findeisen added a comment - Christopher Tubbs , that's good! Then I don't know what problems would shading of Guava introduce for projects using Accumulo. (And i think I know what problems will it solve ) As it is now, I can't use Accumulo and Guava 22. This is because Accumulo uses Guava 14 and e.g HostAndPort class (marked @Beta in that Guava 14) and Guava decided to deprecate and later remove getHostText() method. According to Guava docs: https://github.com/google/guava/wiki/PhilosophyExplained#beta-apis , if you use Beta APIs and you are a library, you should shade (repackage) the Guava dependency. In fact, even if you do not use Beta APIs, then you should still upgrade quite regularly ( https://github.com/google/guava/wiki/PhilosophyExplained#non-beta-apis ). Since Guava 14 they had enough time to deprecate and remove non-Beta APIs too.
          Hide
          kturner Keith Turner added a comment -

          Piotr Findeisen if we were to remove any usage of guava Beta or deprecated methods, do you think that would solve your problem? That would be the path I would like to take. We will need to do it to upgrade our guava version anyway. Maybe this could be done for 1.7.4 and 1.8.2, depending on how disruptive it is.

          Show
          kturner Keith Turner added a comment - Piotr Findeisen if we were to remove any usage of guava Beta or deprecated methods, do you think that would solve your problem? That would be the path I would like to take. We will need to do it to upgrade our guava version anyway. Maybe this could be done for 1.7.4 and 1.8.2, depending on how disruptive it is.
          Hide
          findepi Piotr Findeisen added a comment -

          Keith Turner short therm – if you become compatible with Guava 23 or 22, then yes.

          Long therm – being a library and using (non-shaded) Guava is kind of a commitment that you upgrade regularly. This path is a very good path, as long as you stay on it

          Show
          findepi Piotr Findeisen added a comment - Keith Turner short therm – if you become compatible with Guava 23 or 22, then yes. Long therm – being a library and using (non-shaded) Guava is kind of a commitment that you upgrade regularly. This path is a very good path, as long as you stay on it
          Hide
          milleruntime Michael Miller added a comment -

          I did some quick searches on the code and we only have 28 occurrences of getHostText() in 1.7. Since there is an easy replacement for this method, I will create a ticket and replace it. I don't know if this well help your problem Piotr Findeisen but either way we shouldn't be using Beta methods.

          Show
          milleruntime Michael Miller added a comment - I did some quick searches on the code and we only have 28 occurrences of getHostText() in 1.7. Since there is an easy replacement for this method, I will create a ticket and replace it. I don't know if this well help your problem Piotr Findeisen but either way we shouldn't be using Beta methods.
          Hide
          kturner Keith Turner added a comment -

          even if you do not use Beta APIs, then you should still upgrade quite regularly

          One possible way forward is that for 2.0.0 we jump to the latest Guava. For 1.7.4 and 1.8.2 we maintain the same version of Guava but avoid using methods that don't exists in current Guava.

          We also need to attempt to remove any usage of Beta methods. IIRC the Guava beta annotation is compile time only (its not carried forward into the guava jars), making automated analysis to find usage of Beta stuff difficult.

          Show
          kturner Keith Turner added a comment - even if you do not use Beta APIs, then you should still upgrade quite regularly One possible way forward is that for 2.0.0 we jump to the latest Guava. For 1.7.4 and 1.8.2 we maintain the same version of Guava but avoid using methods that don't exists in current Guava. We also need to attempt to remove any usage of Beta methods. IIRC the Guava beta annotation is compile time only (its not carried forward into the guava jars), making automated analysis to find usage of Beta stuff difficult.
          Hide
          kturner Keith Turner added a comment -

          being a library and using (non-shaded) Guava is kind of a commitment that you upgrade regularly.

          I agree. I opened ACCUMULO-4703 as I think this is a more general issue that needs investigation.

          Show
          kturner Keith Turner added a comment - being a library and using (non-shaded) Guava is kind of a commitment that you upgrade regularly. I agree. I opened ACCUMULO-4703 as I think this is a more general issue that needs investigation.
          Hide
          findepi Piotr Findeisen added a comment -

          For 1.7.4 and 1.8.2 we maintain the same version of Guava but avoid using methods that don't exists in current Guava.

          Keith Turner, that will probably work. Of course, you can also upgrade the dependency in minor releases (assuming Guava is internal and pretty stable, it shouldn't require a major release to bump Guava dependency). Simply upgrading might be simply easier

          BTW @Beta has class file retention. Do you know static analysis tool that could report such usages?

          I did some quick searches on the code and we only have 28 occurrences of getHostText() in 1.7. [...] I don't know if this well help your problem Piotr Findeisen

          Michael Miller, I don't know either. This is the method I found, but I wasn't using source (only jar), so it was a bit harder to check for others. Probably simplest way to check that is locally bumping Guava dependency and making sure code still compiles. (I know this is only an approximation of a formally correct process, but a good one.)

          Show
          findepi Piotr Findeisen added a comment - For 1.7.4 and 1.8.2 we maintain the same version of Guava but avoid using methods that don't exists in current Guava. Keith Turner , that will probably work. Of course, you can also upgrade the dependency in minor releases (assuming Guava is internal and pretty stable, it shouldn't require a major release to bump Guava dependency). Simply upgrading might be simply easier BTW @Beta has class file retention. Do you know static analysis tool that could report such usages? I did some quick searches on the code and we only have 28 occurrences of getHostText() in 1.7. [...] I don't know if this well help your problem Piotr Findeisen Michael Miller , I don't know either. This is the method I found, but I wasn't using source (only jar), so it was a bit harder to check for others. Probably simplest way to check that is locally bumping Guava dependency and making sure code still compiles. (I know this is only an approximation of a formally correct process, but a good one.)
          Hide
          ctubbsii Christopher Tubbs added a comment -

          Piotr Findeisen, other than the misuse of getHostText, which we definitely shouldn't be using, and should replace (ACCUMULO-4702), are there any other problems with Guava compatibility, which cannot be fixed by a user manipulation of the classpath to insert their required version?

          Show
          ctubbsii Christopher Tubbs added a comment - Piotr Findeisen , other than the misuse of getHostText , which we definitely shouldn't be using, and should replace ( ACCUMULO-4702 ), are there any other problems with Guava compatibility, which cannot be fixed by a user manipulation of the classpath to insert their required version?
          Hide
          vines John Vines added a comment -

          Is there a reason we're jumping down the path of the short-sighted solution instead of just shading guava? The only explanation I'm seeing for not is Chris's original justification, which seems to be conflating shading with fatjar-ing.

          Show
          vines John Vines added a comment - Is there a reason we're jumping down the path of the short-sighted solution instead of just shading guava? The only explanation I'm seeing for not is Chris's original justification, which seems to be conflating shading with fatjar-ing.
          Hide
          ctubbsii Christopher Tubbs added a comment -

          John Vines Any form of bundling (shading is a form of bundling, with API relocation) introduces a whole class of problems (which we can discuss on the mailing list if you like). If, as a project, we were to start doing this, I think it would require more careful consideration, discussion, and debate (and the mailing lists are more suited to that than the JIRA comments section).

          ACCUMULO-4702 takes a much more conservative approach, by simply avoiding unstable Guava APIs (those marked "Beta"), which we always should have been avoiding. This is not a "short-sighted" solution, but a cautious and conservative one, which has more limited implications than simply shading Guava.

          Show
          ctubbsii Christopher Tubbs added a comment - John Vines Any form of bundling (shading is a form of bundling, with API relocation) introduces a whole class of problems (which we can discuss on the mailing list if you like). If, as a project, we were to start doing this, I think it would require more careful consideration, discussion, and debate (and the mailing lists are more suited to that than the JIRA comments section). ACCUMULO-4702 takes a much more conservative approach, by simply avoiding unstable Guava APIs (those marked "Beta"), which we always should have been avoiding. This is not a "short-sighted" solution, but a cautious and conservative one, which has more limited implications than simply shading Guava.

            People

            • Assignee:
              ctubbsii Christopher Tubbs
              Reporter:
              findepi Piotr Findeisen
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development