GH-50000: [C++][FlightRPC] Use grpcpp/grpcpp.h not grpcpp/version_info.h for old gRPC

[rok]

Rationale for this change

gRPC 1.51.0 introduced grpcpp/version_info.h. Since we also use older versions we should guard against import in case of older version.

What changes are included in this PR?

Use grpcpp/grpcpp.h not grpcpp/version_info.h. grpcpp/grpcpp.h is provided by old gRPC.

Are these changes tested?

By CI.

Are there any user-facing changes?

No.

• GitHub Issue: [C++][FlightRPC] <grpcpp/version_info.h> not found #50000

[github-actions]

⚠️ GitHub issue #50000 has been automatically assigned in GitHub to PR creator.

[raulcd]

I am wondering whether this is better than a guard as we have on other places:

#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)))))

#if GRPC_CPP_VERSION_CHECK(1, 80, 0)
#  include <grpcpp/version_info.h>
#endif

[rok]

GRPC_CPP_VERSION_MAJOR/MINOR/PATCH are defined inside <grpcpp/version_info.h>. Without importing <grpcpp/version_info.h> the if GRPC_CPP_VERSION_CHECK(1, 80, 0) check will (I believe) always evaluate to 0 and not really do much.

[kou]

Oh, sorry...

[kou]

It seems that grpcpp/version_info.h is available since gRPC 1.51.0: grpc/grpc@8696d49

[rok]

Ah, sorry. We seem to be using 1.30.2 in this case (row 499).

[kou]

How about this?

diff --git a/cpp/src/arrow/flight/transport/grpc/util_internal.h b/cpp/src/arrow/flight/transport/grpc/util_internal.h
index 6ff3dadb53..7e306d574a 100644
--- a/cpp/src/arrow/flight/transport/grpc/util_internal.h
+++ b/cpp/src/arrow/flight/transport/grpc/util_internal.h
@@ -17,18 +17,23 @@
 
 #pragma once
 
-#include <grpcpp/version_info.h>
+#include <grpcpp/grpcpp.h>
 
 #include "arrow/flight/transport/grpc/protocol_grpc_internal.h"
 #include "arrow/flight/types.h"
 #include "arrow/flight/visibility.h"
 #include "arrow/util/macros.h"
 
-#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)))))
+// gRPC 1.51.0 or later defines GRPC_CPP_VERSION_MAJOR and so on.
+#ifdef GRPC_CPP_VERSION_MAJOR
+#  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)))))
+#else
+#  define GRPC_CPP_VERSION_CHECK(major, minor, patch) 0
+#endif
 
 #if GRPC_CPP_VERSION_CHECK(1, 80, 0)
 #  include <absl/status/status.h>

[rok]

Thanks @kou, I love the explicit GRPC_CPP_VERSION_CHECK(major, minor, patch) 0! Applied your suggestion.

[kou]

It's the idea by @spotaws in a0ff085 . :-)

[kou]

+1

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit cca41a3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 14 possible false positives for unstable benchmarks that are known to sometimes produce them.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit cca41a3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 14 possible false positives for unstable benchmarks that are known to sometimes produce them.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit cca41a3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 14 possible false positives for unstable benchmarks that are known to sometimes produce them.