Copilot commented on code in PR #50407:
URL: https://github.com/apache/arrow/pull/50407#discussion_r4216028229
##########
cpp/src/arrow/flight/server.cc:
##########
@@ -159,66 +70,61 @@ Status FlightServerBase::Init(const FlightServerOptions&
options) {
ARROW_ASSIGN_OR_RAISE(impl_->transport_,
internal::GetDefaultTransportRegistry()->MakeServer(
scheme, this, options.memory_manager));
- return impl_->transport_->Init(options, *options.location.uri_);
+ ARROW_ASSIGN_OR_RAISE(auto uri, options.location.uri());
+ return impl_->transport_->Init(options, uri);
}
-int FlightServerBase::port() const { return location().uri_->port(); }
+int FlightServerBase::port() const { return location().port(); }
Location FlightServerBase::location() const { return
impl_->transport_->location(); }
Status FlightServerBase::SetShutdownOnSignals(const std::vector<int> sigs) {
- impl_->signals_ = sigs;
- impl_->old_signal_handlers_.clear();
- return Status::OK();
+ return impl_->signal_state_.SetShutdownOnSignals(sigs);
}
Status FlightServerBase::Serve() {
- if (!impl_->transport_) {
- return Status::UnknownError("Server did not start properly");
- }
- impl_->got_signal_ = 0;
- impl_->old_signal_handlers_.clear();
- impl_->running_instance_ = impl_.get();
-
- ServerSignalHandler signal_handler;
- ARROW_ASSIGN_OR_RAISE(impl_->self_pipe_,
signal_handler.Init(&Impl::WaitForSignals));
- // Override existing signal handlers with our own handler so as to stop the
server.
- for (size_t i = 0; i < impl_->signals_.size(); ++i) {
- int signum = impl_->signals_[i];
- SignalHandler new_handler(&Impl::HandleSignal), old_handler;
- ARROW_ASSIGN_OR_RAISE(old_handler, SetSignalHandler(signum, new_handler));
- impl_->old_signal_handlers_.push_back(std::move(old_handler));
- }
-
- RETURN_NOT_OK(impl_->transport_->Wait());
- impl_->running_instance_ = nullptr;
-
- // Restore signal handlers
- for (size_t i = 0; i < impl_->signals_.size(); ++i) {
- RETURN_NOT_OK(
- SetSignalHandler(impl_->signals_[i],
impl_->old_signal_handlers_[i]).status());
- }
- return Status::OK();
+ return impl_->signal_state_.Serve(
+ [this]() -> Status {
+ if (!impl_->transport_) {
+ return Status::UnknownError("Server did not start properly");
+ }
+ return impl_->transport_->Wait();
+ },
+ [this](const std::chrono::system_clock::time_point* deadline) -> Status {
+ if (!impl_->transport_) {
+ return Status::Invalid("Shutdown() on uninitialized
FlightServerBase");
+ }
+ if (deadline) {
+ return impl_->transport_->Shutdown(*deadline);
+ }
+ return impl_->transport_->Shutdown();
+ },
+ "Server did not start properly", "Error shutting down server");
}
-int FlightServerBase::GotSignal() const { return impl_->got_signal_; }
+int FlightServerBase::GotSignal() const { return
impl_->signal_state_.GotSignal(); }
Status FlightServerBase::Shutdown(const std::chrono::system_clock::time_point*
deadline) {
- auto server = impl_->transport_.get();
- if (!server) {
- return Status::Invalid("Shutdown() on uninitialized FlightServerBase");
- }
- impl_->running_instance_ = nullptr;
- if (deadline) {
- return impl_->transport_->Shutdown(*deadline);
- }
- return impl_->transport_->Shutdown();
+ return impl_->signal_state_.Shutdown(
Review Comment:
`ServerSignalState::Shutdown` clears the process-wide `running_instance_`
before invoking the callback. The callback explicitly returns an error for an
uninitialized `FlightServerBase`, so calling `Shutdown()` on an uninitialized
object while another server is serving disables signal delivery for the active
server; validate the transport before entering this helper (or clear the
instance only after successful validation).
##########
cpp/src/arrow/flight/transport/grpc/util_internal.h:
##########
@@ -51,6 +35,12 @@ class Status;
namespace flight {
+#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)))))
Review Comment:
The version check is now evaluated in the order each translation unit
happens to include headers. In particular, `util_internal.cc` includes this
header before `<grpcpp/grpcpp.h>`, while `grpc_client.cc` includes gRPC first;
with gRPC 1.80+ this can declare `FromAbslStatus` in one TU but omit its
definition in another, causing a link failure. Make the version-info
include/order part of this header and retain the fallback for gRPC versions
without these macros.
--
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]
