[GitHub] [arrow] lidavidm commented on a diff in pull request #36009: WIP: GH-35500: [C++][Go][FlightRPC] Add support for result set expiration

["lidavidm (via GitHub)" <gi...@apache.org>]

lidavidm commented on code in PR #36009:
URL: https://github.com/apache/arrow/pull/36009#discussion_r1224357589


##########
dev/archery/archery/integration/runner.py:
##########
@@ -435,6 +435,37 @@ def run_all_tests(with_cpp=True, with_java=True, with_js=True,
             description="Ensure FlightInfo.ordered is supported.",
             skip={"JS", "C#", "Rust"},
         ),
+        Scenario(
+            "expiration_time:do_get",
+            description=("Ensure FlightEndpoint.expiration_time with "
+                         "DoGet is working as expected."),
+            skip={"Java", "Go", "JS", "C#", "Rust"},
+        ),
+        Scenario(
+            "expiration_time:list_actions",
+            description=("Ensure FlightEndpoint.expiration_time related "
+                         "pre-defined actions is working with ListActions "
+                         "as expected."),
+            skip={"Java", "Go", "JS", "C#", "Rust"},
+        ),
+        Scenario(
+            "expiration_time:cancel_flight_info",
+            description=("Ensure FlightEndpoint.expiration_time and "
+                         "CancelFlightInfo are working as expected."),
+            skip={"Java", "Go", "JS", "C#", "Rust"},
+        ),
+        Scenario(
+            "expiration_time:refresh_flight_info",
+            description=("Ensure FlightEndpoint.expiration_time and "
+                         "RefreshFlightEndpoint are working as expected."),
+            skip={"Java", "Go", "JS", "C#", "Rust"},
+        ),
+        Scenario(
+            "expiration_time:close_flight_info",
+            description=("Ensure FlightEndpoint.expiration_time and "
+                         "CloseFlightInfo are working as expected."),
+            skip={"Java", "Go", "JS", "C#", "Rust"},

Review Comment:
   I can work on Java next week



##########
cpp/src/arrow/flight/sql/client.h:
##########
@@ -324,6 +324,7 @@ class ARROW_FLIGHT_SQL_EXPORT FlightSqlClient {
   Status Rollback(const FlightCallOptions& options, const Savepoint& savepoint);
 
   /// \brief Explicitly cancel a query.
+  /// Deprecated since 13.0.0. Use FlightClient::CancelFlightInfo() instead.

Review Comment:
   Doxygen has a [`\deprecated`](https://www.doxygen.nl/manual/commands.html#cmddeprecated) command



##########
format/Flight.proto:
##########
@@ -321,6 +348,13 @@ message FlightEndpoint {
    * represent redundant and/or load balanced services.
    */
   repeated Location location = 2;
+
+  /*
+   * Expiration time of this stream. If present, clients may assume
+   * they can retry DoGet requests. Otherwise, clients should avoid
+   * retrying DoGet requests.

Review Comment:
   I wonder if wording it as "it is application-defined whether DoGet requests may be retried" might be better since technically that is how it was before



##########
cpp/src/arrow/flight/client.cc:
##########
@@ -569,6 +569,36 @@ Status FlightClient::DoAction(const FlightCallOptions& options, const Action& ac
   return DoAction(options, action).Value(results);
 }
 
+arrow::Result<std::unique_ptr<ActionCancelFlightInfoResult>>
+FlightClient::CancelFlightInfo(const FlightCallOptions& options, const FlightInfo& info) {
+  ARROW_ASSIGN_OR_RAISE(auto body, info.SerializeToString());
+  Action action{ActionType::kCancelFlightInfo.type, Buffer::FromString(body)};
+  ARROW_ASSIGN_OR_RAISE(auto stream, DoAction(options, action));
+  ARROW_ASSIGN_OR_RAISE(auto result, stream->Next());

Review Comment:
   We should drain the rest of the DoAction stream since that's the only way we can get the server error (if any). You can see how this is done in the Flight SQL codebase: https://github.com/apache/arrow/blob/766e25440250dde9117e19245389badb5ed7678c/cpp/src/arrow/flight/sql/client.cc#L131-L137



##########
cpp/src/arrow/flight/serialization_internal.cc:
##########
@@ -147,6 +169,15 @@ Status ToProto(const FlightEndpoint& endpoint, pb::FlightEndpoint* pb_endpoint)
   for (const Location& location : endpoint.locations) {
     RETURN_NOT_OK(ToProto(location, pb_endpoint->add_location()));
   }
+  if (endpoint.expiration_time.has_value()) {
+    const auto expiration_time = endpoint.expiration_time.value();
+    const auto since_epoch = expiration_time.time_since_epoch();
+    const auto since_epoch_ns =
+        std::chrono::duration_cast<std::chrono::nanoseconds>(since_epoch).count();
+    auto pb_expiration_time = pb_endpoint->mutable_expiration_time();
+    pb_expiration_time->set_seconds(since_epoch_ns / 1000000000);

Review Comment:
   nit: constant for the nanoseconds to seconds conversion? (Do we already have one?)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org