Upgrade to airline 2.x

[rguillome]

Test plan - Existing TU and launch Presto Spark Launcher and Presto Cli from local environment

Depends on https://github.com/facebookexternal/presto-facebook/pull/1840

This migration remove deprecated dependency to airline 0.x version and bring new features as allowing two options to be mutualy exclusive.

Fix this issue #16236

== NO RELEASE NOTE ==

[zacw7]

Thanks for the contribution. But Presto maintains its own airline and I don't think we are planning to adopt any forked version.

[rguillome]

Hi @zacw7,

I created one year ago, an issue on presto's airline but one maintainer told me this version would be deprecated soon.
As I understood there is a conflict between 2 fork and maybe this maintainer didn't speak in the name of the presto team. Could you confirm that after looking at the issue and this comment.

If you confirm that it won't be a waste of time, I would make a PR to the presto's airline version to add the mutually exclusive option.

[zacw7]

Thanks for the explanations. Is the mutually exclusive option for presto-on-spark?

[rguillome]

Yes, this mutually exclusive option is for presto-on-spark.
We need to pass a new argument to the command : query instead of file because we want to pass the query directly from the command line (see the PR #16249) in an AWS EMR use case. For now, the related PR is not convinient because we can't mutually exclude passing the argument file or query to the presto-on-spark command line.

[zacw7]

@aweisberg Ariel, do you mind taking a look and providing some insights from Presto-on-Spark perspective? Thanks!

[aweisberg]

Thanks for the contribution. But Presto maintains its own airline and I don't think we are planning to adopt any forked version.

Oddly enough it looks like we never deployed the forked version? I think it would be fine to upgrade assuming no bugs/compatibility issues. It's just going from one version of the dependency we don't own to another.

I'll do a real review tomorrow.

[rguillome]

Hi @aweisberg

Are changes ok for you ?

[aweisberg]

Sorry been very busy. At a high level yes, but I need to test it. I have a hard time knowing what these ignored class patterns are about, and the exclusion for Jakarta and whether it is safe. Nothing ever seems safe with Java dependencies.

[rguillome]

Thanks for the feedback !

Yes I should commit these ignored-class-patterns appart in a separate commit in order to indicate why they were mandatory. I didn't find a better way to avoid duplicate-finder-maven-plugin complaining about the "file" META-INF.versions.9.module-info present twice in airline-io and airline jars.

Excluding Jakarta is for the same reason: to avoid duplication of several classes between the new "Jakarta" jar vs the old-javax one. Jakarta had a bad time (before its 2.x version) where they kept the same package name as the javax one.
At the end, I chose to had as less impact as possible with the existant so keeping only the javax.inject:javax.inject jars seemed to be the safer solution.
But I 'm going to ask airline why they don't migrate on jakarta 2.x. Using a different package name should avoid to exclude the jakarta jar from the airline dependency.

[aweisberg]

META-INF.versions.9.module-info seems to be an issue with the duplicate finder plugin. Instead of putting the exclusion in everywhere it would be better to add it to the root pom or even the base pom in airlift.

It looks like there are already quite a few https://github.com/prestodb/airbase/blob/master/airbase/pom.xml
FYI we have our own airbase fork that I just linked to.

[aweisberg]

Created prestodb/airbase#20 to add the duplicate-finder exclusion to airbase. We can remove it from the Presto poms.

[aweisberg]

Airbase 103 is out now.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: rguillome / name: Guillaume Renaudin (782ce9597e69582621b6d71f1ef5150b77df88e1, d85be1c2d4ccce3d9d7bc5af92cff9e62603f7ab)

[rguillome]

@aweisberg this is updated. I just kept the exclusion on presto-spark-testing/pom.xml since it needs a redefinition of the duplicate-finder-plugin.

[aweisberg]

Can you clean up the history? So first commit should just be upgrade airbase, second commit would be the functional upgrade to airline 2.x. Then I can approve.

[rguillome]

It's done @aweisberg !

[aweisberg]

LGTM, thank you!

I don't think this needs a release note because it isn't a user visible change.

[rschlussel]

Can you test that there are no changes with --help for the cli? Looks like the way that is generated has changed a bit with airline 2.x, so want to confirm that it still works as expected.

[rguillome]

Hi @rschlussel

I checked differences between my branch and the current master one for presto-cli --help and presto-spark-launcher --help

As I can see, the only differences are :

• some options are not at the same line. For example, on master for presto-cli --help, I see two lines for

                [--client-tags <client tags>] [--debug] [--disable-compression]

                [--execute <execute>] [--extra-credential <extra-credential>...]
                [(-f <file> | --file <file>)] [(-h | --help)]

while on this migration-to-airlift-2 branch, I see 4 lines

                [ --client-tags <client tags> ] [ --debug ]
                [ --disable-compression ] [ --execute <execute> ]
                [ --extra-credential <extra-credential>... ]
                [ {-f | --file} <file> ] [ {-h | --help} ]

But there are exactly the same number of options with the same names

• Space are added after the opening square brackets and before the closing square brackets. Ex :
  [--client-tags <client tags>] => [ --client-tags <client tags> ]
• brackets are replaced by curl brackets when having choices for option Ex:
  [(-h | --help)]=> [ {-h | --help} ]
• on presto-spark-launcher there is a change, in the master version, the -c option is required in the help, while in the airline2 it's coherent with the others options : there are indicated as optional
  (-c <file> | --config <file>) => [ {-c | --config} <file> ]
  That brings up the point : why all required arguments are indicated as optional by the help ? The following example indicates that it should appears without square brackets in the help : https://rvesse.github.io/airline/guide/restrictions/index.html (breaking with the current CLI help message)

[rschlussel]

The multiple lines thing is fine, but would be good to fix the issue with required arguments not showing up correctly.

[elharo]

stale, closing due to conflicts

[rguillome]

It could have been better to ask for a rebase and maybe to integrate the fix about required arguments than closing the PR. I think upgrading to airline 2.x should be a good move for prestodb !

[tdcmeehan]

@rguillome please feel free to reopen after you have fixed the merge conflicts and incorporate @rschlussel's suggestions, we'll be happy to review once you do.