> > Hence I'll merge it unless there's objections tomorrow afternoon (13 EST).
I left a couple of minor comments. On Thu, Oct 8, 2020 at 6:42 PM David Li <li.david...@gmail.com> wrote: > Wes has taken a look and so have I now, and I think this should be OK. > I know Krisztián preferred to defer it before, but now it does not > bump the gRPC version everywhere to pass the tests. > > Hence I'll merge it unless there's objections tomorrow afternoon (13 EST). > > David > > On 10/8/20, Antoine Pitrou <anto...@python.org> wrote: > > > > 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. > >>>>>> > >>>>> > >>> > >> > >> > > >
