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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
