Copilot commented on code in PR #13384:
URL: https://github.com/apache/gluten/pull/13384#discussion_r4325517728
##########
cpp/velox/compute/VeloxRuntime.cc:
##########
@@ -284,6 +283,12 @@ void VeloxRuntime::initializeExecutors() {
}
void VeloxRuntime::registerConnectors() {
+ // Connectors are registered lazily when the first stage of this runtime
starts, so that stage-level
+ // information is available when creating them. They stay registered until
the runtime is destroyed.
+ if (connectorsRegistered_) {
+ return;
+ }
+ connectorsRegistered_ = true;
Review Comment:
As written, `connectorsRegistered_` is a plain `bool` checked/updated
without synchronization. If multiple threads can call `createResultIterator()`
concurrently on the same runtime, this introduces a data race and can still
lead to double-registration. Use a thread-safe one-time initialization
mechanism (e.g., `std::once_flag` + `std::call_once`, or guard with a mutex; if
using atomic, pair it with correct ordering and still ensure registration
itself is serialized).
##########
cpp/velox/compute/VeloxRuntime.cc:
##########
@@ -284,6 +283,12 @@ void VeloxRuntime::initializeExecutors() {
}
void VeloxRuntime::registerConnectors() {
+ // Connectors are registered lazily when the first stage of this runtime
starts, so that stage-level
+ // information is available when creating them. They stay registered until
the runtime is destroyed.
+ if (connectorsRegistered_) {
+ return;
+ }
+ connectorsRegistered_ = true;
auto* backend = VeloxBackend::get();
connectorIds_.hiveRegistered =
velox::connector::registerConnector(backend->createHiveConnector(connectorIds_.hive,
ioExecutor_.get()));
Review Comment:
`connectorsRegistered_` is set to `true` before the connector registration
calls execute. If any registration throws/fails, the runtime can end up in a
state where connectors are not fully registered but future calls will
early-return and never retry. Set the flag only after successful completion (or
use `std::call_once` so failures don’t permanently wedge the state).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
