You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@arrow.apache.org by James Duong <ja...@bitquilltech.com> on 2020/10/08 16:43:00 UTC
Flight gRPC version and disabling server verification in C++ [was Re:
2.0.0 release timeline: October 9]
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 <sz...@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 <em...@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.richardson@gmail.com>
> > wrote:
> >
> > > On Wed, Oct 7, 2020 at 1:11 PM Antoine Pitrou <an...@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.
> > > >
> > >
>
--
*James Duong*
Lead Software Developer
Bit Quill Technologies Inc.
Direct: +1.604.562.6082 | jamesd@bitquilltech.com
https://www.bitquilltech.com
This email message is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. Any unauthorized review,
use, disclosure, or distribution is prohibited. If you are not the
intended recipient, please contact the sender by reply email and destroy
all copies of the original message. Thank you.
Re: Flight gRPC version and disabling server verification in C++ [was
Re: 2.0.0 release timeline: October 9]
Posted by Micah Kornfield <em...@gmail.com>.
>
> 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...@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 <an...@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
> >> <sz...@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 <emkornfield@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.richardson@gmail.com>
> >>>> wrote:
> >>>>
> >>>>> On Wed, Oct 7, 2020 at 1:11 PM Antoine Pitrou <an...@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.
> >>>>>>
> >>>>>
> >>>
> >>
> >>
> >
>
Re: Flight gRPC version and disabling server verification in C++ [was
Re: 2.0.0 release timeline: October 9]
Posted by David Li <li...@gmail.com>.
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 <an...@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
>> <sz...@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 <em...@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.richardson@gmail.com>
>>>> wrote:
>>>>
>>>>> On Wed, Oct 7, 2020 at 1:11 PM Antoine Pitrou <an...@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.
>>>>>>
>>>>>
>>>
>>
>>
>
Re: Flight gRPC version and disabling server verification in C++ [was
Re: 2.0.0 release timeline: October 9]
Posted by Antoine Pitrou <an...@python.org>.
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 <sz...@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 <em...@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.richardson@gmail.com>
>>> wrote:
>>>
>>>> On Wed, Oct 7, 2020 at 1:11 PM Antoine Pitrou <an...@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.
>>>>>
>>>>
>>
>
>