GH-49661: [CI][C++] Suppress deprecated warnings with gRPC 1.80.0#49662
GH-49661: [CI][C++] Suppress deprecated warnings with gRPC 1.80.0#49662kou merged 1 commit intoapache:mainfrom
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR updates Arrow Flight’s gRPC C++ transport TLS setup to accommodate gRPC 1.80.0 API changes and suppress new deprecation warnings.
Changes:
- Add a gRPC C++ version-check macro used to switch behavior for gRPC >= 1.80.0.
- Introduce
FromAbslStatusto convert Abseil status results into ArrowStatus. - Update the TLS credentials setup path to use the newer certificate provider API and avoid deprecated calls on gRPC >= 1.80.0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cpp/src/arrow/flight/transport/grpc/util_internal.h | Adds gRPC version-check macro and conditionally declares FromAbslStatus. |
| cpp/src/arrow/flight/transport/grpc/util_internal.cc | Implements Abseil-to-Arrow status conversion helper. |
| cpp/src/arrow/flight/transport/grpc/grpc_client.cc | Switches TLS certificate provider setup depending on gRPC version and updates credential option calls accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define GRPC_CPP_VERSION_CHECK(major, minor, patch) \ | ||
| ((GRPC_CPP_VERSION_MAJOR > (major) || \ | ||
| (GRPC_CPP_VERSION_MAJOR == (major) && GRPC_CPP_VERSION_MINOR > (minor)) || \ | ||
| ((GRPC_CPP_VERSION_MAJOR == (major) && GRPC_CPP_VERSION_MINOR == (minor) && \ | ||
| GRPC_CPP_VERSION_PATCH >= (patch))))) |
There was a problem hiding this comment.
GRPC_CPP_VERSION_CHECK relies on GRPC_CPP_VERSION_{MAJOR,MINOR,PATCH} but this header doesn’t include the header that defines them. Please include the appropriate gRPC version header (e.g. grpcpp/version_info.h) or add a defensive #ifdef GRPC_CPP_VERSION_MAJOR fallback to avoid brittle transitive-include dependencies.
| // Abseil is included. | ||
| #ifdef ABSL_NAMESPACE_BEGIN | ||
| /// Convert a Abseil status to an Arrow status. | ||
| ARROW_FLIGHT_EXPORT | ||
| Status FromAbslStatus(const ::absl::Status& absl_status); |
There was a problem hiding this comment.
FromAbslStatus uses ::absl::Status/::absl::StatusCode but neither this header nor the corresponding .cc includes the Abseil status header. Relying on indirect includes is fragile and can break IWYU builds; please explicitly include <absl/status/status.h> (guarded with __has_include if needed).
|
|
||
| // Abseil is included. | ||
| #ifdef ABSL_NAMESPACE_BEGIN | ||
| /// Convert a Abseil status to an Arrow status. |
There was a problem hiding this comment.
Grammar: “Convert a Abseil status” should be “Convert an Abseil status”.
| /// Convert a Abseil status to an Arrow status. | |
| /// Convert an Abseil status to an Arrow status. |
| # if GRPC_CPP_VERSION_CHECK(1, 80, 0) | ||
| auto certificate_provider = | ||
| std::make_shared<::grpc::experimental::InMemoryCertificateProvider>(); | ||
| RETURN_NOT_OK(FromAbslStatus(certificate_provider->UpdateRoot(kDummyRootCert))); | ||
| # else |
There was a problem hiding this comment.
This code calls FromAbslStatus(...), but FromAbslStatus is only declared when ABSL_NAMESPACE_BEGIN is defined. Please either (1) ensure Abseil status headers are included so the symbol is always available in this build configuration, or (2) guard this call with the same preprocessor condition to avoid compilation failures in builds where Abseil isn’t present/used.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // gRPC 1.80.0 or later use absl::Status. | ||
| #if GRPC_CPP_VERSION_CHECK(1, 80, 0) | ||
| /// Convert an Abseil status to an Arrow status. | ||
| ARROW_FLIGHT_EXPORT | ||
| Status FromAbslStatus(const ::absl::Status& absl_status); | ||
| #endif |
There was a problem hiding this comment.
FromAbslStatus is declared with ::absl::Status, but this header doesn’t include an Abseil status header or forward-declare absl::Status. To avoid relying on transitive includes, please add an appropriate include (e.g., Abseil status header) or a forward declaration for absl::Status guarded by the same version check.
| #if GRPC_CPP_VERSION_CHECK(1, 80, 0) | ||
| Status FromAbslStatus(const ::absl::Status& absl_status) { | ||
| switch (absl_status.code()) { | ||
| case ::absl::StatusCode::kOk: | ||
| return Status::OK(); | ||
| case ::absl::StatusCode::kCancelled: | ||
| return Status::Cancelled(absl_status.ToString()); | ||
| case ::absl::StatusCode::kUnknown: | ||
| return Status::UnknownError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kInvalidArgument: | ||
| return Status::Invalid(absl_status.ToString()); | ||
| case ::absl::StatusCode::kDeadlineExceeded: | ||
| return Status::IOError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kNotFound: | ||
| return Status::KeyError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kAlreadyExists: | ||
| return Status::AlreadyExists(absl_status.ToString()); | ||
| case ::absl::StatusCode::kPermissionDenied: | ||
| return Status::IOError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kResourceExhausted: | ||
| return Status::IOError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kFailedPrecondition: | ||
| return Status::IOError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kAborted: | ||
| return Status::IOError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kOutOfRange: | ||
| return Status::Invalid(absl_status.ToString()); | ||
| case ::absl::StatusCode::kUnimplemented: | ||
| return Status::NotImplemented(absl_status.ToString()); | ||
| case ::absl::StatusCode::kInternal: | ||
| return Status::IOError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kUnavailable: | ||
| return Status::IOError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kDataLoss: | ||
| return Status::IOError(absl_status.ToString()); | ||
| case ::absl::StatusCode::kUnauthenticated: | ||
| return Status::IOError(absl_status.ToString()); | ||
| default: | ||
| return Status::UnknownError(absl_status.ToString()); | ||
| } |
There was a problem hiding this comment.
This file uses ::absl::Status / ::absl::StatusCode in FromAbslStatus but doesn’t directly include the Abseil status header. Please include the defining header here as well (rather than relying on gRPC’s transitive includes) to make the build more robust across gRPC versions/packaging.
| # if GRPC_CPP_VERSION_CHECK(1, 80, 0) | ||
| auto certificate_provider = | ||
| std::make_shared<::grpc::experimental::InMemoryCertificateProvider>(); | ||
| RETURN_NOT_OK(FromAbslStatus(certificate_provider->UpdateRoot(kDummyRootCert))); | ||
| # else | ||
| auto certificate_provider = | ||
| std::make_shared<::grpc::experimental::StaticDataCertificateProvider>( | ||
| kDummyRootCert); |
There was a problem hiding this comment.
PR description says to use grpc::experimental::InMemoryDataCertificateProvider, but the code uses ::grpc::experimental::InMemoryCertificateProvider. Please align the PR description with the actual type used (or update the code if the intended type is different).
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 8eb2ca1. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
….0 (apache#49662) ### Rationale for this change gRPC 1.80.0 introduced some deprecation warnings. See apacheGH-49661 for real warning messages. ### What changes are included in this PR? * Use `grpc::experimental::InMemoryCertificateProvider` instead of `grpc::experimental::StaticDataCertificateProvider` * Use `set_root_certificate_provider()` instead of `set_certificate_provider()` of `grpc::experimental::TlsChannelCredentialsOptions` * Don't call `::grpc::experimental::TlsChannelCredentialsOptions::watch_root_certs()` because we called `set_root_certificate_provider()` with gRPC 1.80.0 or later ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#49661 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Rationale for this change
gRPC 1.80.0 introduced some deprecation warnings. See GH-49661 for real warning messages.
What changes are included in this PR?
grpc::experimental::InMemoryCertificateProviderinstead ofgrpc::experimental::StaticDataCertificateProviderset_root_certificate_provider()instead ofset_certificate_provider()ofgrpc::experimental::TlsChannelCredentialsOptions::grpc::experimental::TlsChannelCredentialsOptions::watch_root_certs()because we calledset_root_certificate_provider()with gRPC 1.80.0 or laterAre these changes tested?
Yes.
Are there any user-facing changes?
No.