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

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

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


##########
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:
   Thanks!
   Suggestions on wording are always welcome. :-)



##########
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:
   Thanks!



##########
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:
   Oh, I didn't notice it.
   Is it OK that we move `DrainResultStream()` and `ReadResult()` to `flight/client.h` and use them in both `flight/client.cc` and `flight/sql/client.cc`?



##########
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:
   Good catch!
   It seems that we can use `std::nano::den`.



##########
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:
   Done!



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