-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-11695: [C++][FlightRPC] fix option to disable TLS verification #9569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
CC @xhochy; I've added a CMake flag to fail the build if a supported gRPC version isn't detected. |
|
Tested with these gRPC versions from conda-forge: 1.36.0 |
faec8f2 to
71b54bc
Compare
|
It looks like https://github.com/apache/arrow/pull/9569/checks?check_run_id=1974857560#step:5:524 is a valid failure but the integration test seems like a flaky test. |
|
Opened grpc/grpc#25556 upstream. |
|
Added the patch to the conda recipe and seems to be working fine: conda-forge/arrow-cpp-feedstock#362 |
71b54bc to
1124853
Compare
|
Pushed a fix for the warning on MacOS. Thanks Antoine! To be fair to them, we're explicitly using things under a grpc::experimental namespace, but the macros would be nice. I want to look at the flaky integration test next - that's covered by ARROW-11717. |
|
Ah, thanks for pointing it out. I had missed the experimental part. |
pitrou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objection from me.
xhochy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, would merge on green.
|
Hmm, it seems MacOS is different about this and I'll need to actually include the dummy root cert still. Let me update the PR. (grpc/grpc#21655 seems related upstream.) |
1124853 to
e768c3c
Compare
|
And now it passes (minus the integration test which I'll look at next). |
As said before: It seems unrelated to me as I saw that failures in an other PR, too. |
Sorry, yes, I mean I want to look at it as my next PR (not in this PR). Apologies for the confusion. |
gRPC 1.34 and 1.36 both change up the API, so we have to detect both of those versions. The CMake config and C++ code was also refactored a bit so that there's less to copy-paste for each gRPC version change.