I'll get to review the PR on Monday. It may be too late for the release.
Regards Antoine. Le 08/10/2020 à 18:43, James Duong a écrit : > Hi, > > I've edited my PR now so that: > 1. The CMakefiles so that we can detect which namespace > TlsCredentialsOptions are in, if any. > 2. Conditionally compile the C++ Flight client to use the namespace to > implement disabling server verification, or compile out the implementation > and throw an error at runtime if it is not available and the user tries to > enable it. > 3. The tests are also conditionally compiled. > > I feel this gives us the best of both worlds: > 1. Users of the newer gRPCs (1.27+) can take advantage of this option and > the namespace is mapped automatically. > 2. Users of gRPC pre-1.27 do not need to upgrade (but will see an error if > they use this option). > 3. This is all transparent to someone building gRPC. There's no need to use > additional macros. > > This has now passed all 36 CI tests, except this travis-ci job that is > still pending: > https://travis-ci.org/github/apache/arrow/builds/734025800 > > Are we comfortable getting this into 2.0.0 with this design? I feel it is > low risk now, especially since upgrading is not mandatory. > > On Wed, Oct 7, 2020 at 4:15 PM Krisztián Szűcs <szucs.kriszt...@gmail.com> > wrote: > >> There is still some work left to make the packaging builds pass on the >> PR. Considering how close we are to the release I find it risky to >> include that change to 2.0. So I'm in favor of postponing it to 3.0. >> >> >> On Wed, Oct 7, 2020 at 11:10 PM Micah Kornfield <emkornfi...@gmail.com> >> wrote: >>> >>> I agree with Antoine that we shouldn't be making changes to dependency >>> versions so close to a release. This is consistent with other types of >>> changes that could have a potentially large blast radius >>> >>> I don't have a strong opinion on what version we end up with though >> (would >>> need to do more research on compatibility guarantees) >>> >>> Micah >>> >>> On Wednesday, October 7, 2020, Neal Richardson < >> neal.p.richard...@gmail.com> >>> wrote: >>> >>>> On Wed, Oct 7, 2020 at 1:11 PM Antoine Pitrou <anto...@python.org> >> wrote: >>>> >>>>> >>>>> Le 07/10/2020 à 21:55, Neal Richardson a écrit : >>>>>> * The only version that is a requirement is >>>>>> >>>>> >>>> >> https://github.com/apache/arrow/pull/8325/files#diff-2420b0c5b6bdad921f1d538f92d64b59R2516 >>>>> , >>>>>> and so that's the one we're concerned about increasing. If we can >> keep >>>> it >>>>>> low with an #ifdef, great. That said, I have no idea how slow >> people >>>> are >>>>> to >>>>>> update gRPC, or even what constitutes "old", so I can't say how >> much >>>>> extra >>>>>> complication it is worth to support old versions. >>>>> >>>>> Well, the gRPC version provided by Ubuntu 20.04 is 1.16.1. >>>>> >>>> >>>> According to >>>> >>>> >> https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L2509 >>>> , >>>> we already require 1.17, which is newer than that. And we've required >> that >>>> for the last year: >>>> >>>> >> https://github.com/apache/arrow/commit/a70cf783364b140cab172e1851b563295c46e333 >>>> >>>> >>>>> >>>>>> * However, provided that the bundled build_grpc cmake macro works >>>> (surely >>>>>> we test that somewhere right?), if someone has >>>>> ARROW_DEPENDENCY_SOURCE=AUTO >>>>>> *and* they have old gRPC on their system, instead of a build >> failure >>>>>> they'll just get a slower build with the bundled grpc included. >> That's >>>>> not >>>>>> a bad experience, and if the user doesn't like it, presumably they >> can >>>>>> upgrade system gRPC and rebuild. >>>>> >>>>> How do you upgrade system gRPC without potentially breaking other >>>>> packages that rely on it? If it's a system library, it's generally >>>>> recommended to follow system-dictated lifecycles. >>>>> >>>>> I am not saying that we should ensure compatibility with antiquated >>>>> versions of gRPC, but being incompatible with the version provided by >>>>> Ubuntu 20.04 (a 6-month old distribution) may be exaggerated. >>>>> >>>>> Regards >>>>> >>>>> Antoine. >>>>> >>>> >> > >
