GH-35377: [C++][FlightRPC] Add a `ServerCallContext` parameter to `arrow::flight::ServerAuthHandler` methods #35378

kou · 2023-05-01T07:18:25Z

Rationale for this change

Because they are also RPC calls to like others such as ListFlights() and DoGet().

What changes are included in this PR?

Add with ServerCallContext versions.

Old signature versions still exist for keeping backward compatibility.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

This PR includes breaking changes to public APIs.

C++ API: Don't include any breaking changes.
C GLib API: Includes a breaking change. context argument is added.
Closes: [C++][FlightRPC] Add a ServerCallContext parameter to arrow::flight::ServerAuthHandler methods #35377

github-actions · 2023-05-01T07:18:45Z

Closes: [C++][FlightRPC] Add a ServerCallContext parameter to arrow::flight::ServerAuthHandler methods #35377

github-actions · 2023-05-01T07:18:47Z

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

…to `arrow::flight::ServerAuthHandler` methods Because they are also RPC calls to like other `ListFlights()`, `DoGet()` and so on.

lidavidm

LGTM.

lidavidm · 2023-05-01T08:03:36Z

cpp/src/arrow/flight/server_auth.h

+  /// If this version is implemented, the deprecated Authentication()
+  /// without ServerCallContext version isn't used. So we can
+  /// implement the deprecated version like the following:
+  ///
+  ///   Status Authenticate(ServerAuthSender* outgoing,
+  ///                       ServerAuthReader* incoming) override {
+  ///     return Status::NotImplemented("This version is never used");
+  ///   }


We could change the deprecated overload from pure virtual to just virtual, and add a default no-op/always-failing implementation. That way people who only implement the new overload won't have to update code when we remove the deprecated overload. What do you think?

Oh, it's a good idea! I'll do it.

lidavidm · 2023-05-01T08:04:23Z

cpp/src/arrow/flight/server_auth.h

+  /// \return Status OK if this authentication is succeeded.
+  /// \deprecated Deprecated since 13.0.0. Implement the Authentication()
+  /// with ServerCallContext version instead.
  virtual Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0;


I suppose we can't mark this as deprecated to the compiler without triggering a bunch of warnings on ourselves, can we...

We can mark this as deprecated but client codes that overrides this don't receive any deprecated warning from their compiler. Because client codes don't call this method. Our server implementations call this method instead.

Anyway, I've added ARROW_DEPRECATED() because a -Wdocumentation-deprecated-sync warning is reported without it: https://github.com/kou/arrow/actions/runs/4849400904/jobs/8641350382#step:9:1437

/Users/runner/work/arrow/arrow/cpp/src/arrow/flight/server_auth.h:83:8: error: declaration is marked with '\deprecated' command but does not have a deprecation attribute [-Werror,-Wdocumentation-deprecated-sync] /// \deprecated Deprecated since 13.0.0. Implement the Authentication() ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/runner/work/arrow/arrow/cpp/src/arrow/flight/server_auth.h:85:3: note: add a deprecation attribute to the declaration to silence this warning virtual Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0; ^

lidavidm · 2023-05-01T11:34:43Z

cpp/src/arrow/flight/server_auth.h

+  /// authentication method supports it.
+  /// \return Status OK if the token is valid, any other status if
+  /// validation failed
+  virtual Status IsValid(const ServerCallContext& context, const std::string& token,


Looks like doxygen complains unless there's a \param for this

Good catch!
I forgot to add it...

ursabot · 2023-05-02T07:22:49Z

Benchmark runs are scheduled for baseline = be12888 and contender = 42527e1. 42527e1 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.66% ⬆️0.06%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.93% ⬆️0.12%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 42527e15 ec2-t3-xlarge-us-east-2
[Failed] 42527e15 test-mac-arm
[Failed] 42527e15 ursa-i9-9960x
[Finished] 42527e15 ursa-thinkcentre-m75q
[Finished] be128889 ec2-t3-xlarge-us-east-2
[Failed] be128889 test-mac-arm
[Failed] be128889 ursa-i9-9960x
[Finished] be128889 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot · 2023-05-02T07:24:16Z

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm

…to `arrow::flight::ServerAuthHandler` methods (apache#35378) ### Rationale for this change Because they are also RPC calls to like others such as `ListFlights()` and `DoGet()`. ### What changes are included in this PR? Add with `ServerCallContext` versions. Old signature versions still exist for keeping backward compatibility. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. **This PR includes breaking changes to public APIs.** * C++ API: Don't include any breaking changes. * C GLib API: Includes a breaking change. `context` argument is added. * Closes: apache#35377 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou requested a review from lidavidm as a code owner May 1, 2023 07:18

github-actions bot added Component: C++ Component: FlightRPC awaiting committer review Awaiting committer review labels May 1, 2023

apacheGH-35377: [C++][FlightRPC] Add a ServerCallContext parameter …

b8bfd16

…to `arrow::flight::ServerAuthHandler` methods Because they are also RPC calls to like other `ListFlights()`, `DoGet()` and so on.

kou force-pushed the cpp-flight-server-auth-handler-call-context branch from 48b71e7 to b8bfd16 Compare May 1, 2023 07:22

lidavidm reviewed May 1, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 1, 2023

Use NotImplemented implementation instead of pure virtual function

4f5428b

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels May 1, 2023

lidavidm reviewed May 1, 2023

View reviewed changes

kou added 2 commits May 2, 2023 06:25

Fix document

36d22f2

GLib: follow changes

cdc11ac

github-actions bot added Component: GLib awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 2, 2023

lidavidm approved these changes May 2, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 2, 2023

kou merged commit 42527e1 into apache:main May 2, 2023

kou deleted the cpp-flight-server-auth-handler-call-context branch May 2, 2023 03:12

GH-35377: [C++][FlightRPC] Add a ServerCallContext parameter to arrow::flight::ServerAuthHandler methods #35378

GH-35377: [C++][FlightRPC] Add a ServerCallContext parameter to arrow::flight::ServerAuthHandler methods #35378

Uh oh!

Conversation

kou commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented May 1, 2023

Uh oh!

github-actions bot commented May 1, 2023

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm May 1, 2023

Choose a reason for hiding this comment

Uh oh!

kou May 1, 2023

Choose a reason for hiding this comment

Uh oh!

kou May 1, 2023

Choose a reason for hiding this comment

Uh oh!

lidavidm May 1, 2023

Choose a reason for hiding this comment

Uh oh!

kou May 1, 2023

Choose a reason for hiding this comment

Uh oh!

lidavidm May 1, 2023

Choose a reason for hiding this comment

Uh oh!

kou May 1, 2023

Choose a reason for hiding this comment

Uh oh!

ursabot commented May 2, 2023

Uh oh!

ursabot commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GH-35377: [C++][FlightRPC] Add a `ServerCallContext` parameter to `arrow::flight::ServerAuthHandler` methods #35378

GH-35377: [C++][FlightRPC] Add a `ServerCallContext` parameter to `arrow::flight::ServerAuthHandler` methods #35378

kou commented May 1, 2023 •

edited

Loading