isapego commented on code in PR #7080:
URL: https://github.com/apache/ignite-3/pull/7080#discussion_r2566764820
##########
modules/platforms/cpp/ignite/client/detail/node_connection.cpp:
##########
@@ -203,7 +208,46 @@ std::shared_ptr<response_handler>
node_connection::find_handler_unsafe(std::int6
if (it == m_request_handlers.end())
return {};
- return it->second;
+ return it->second.handler;
+}
+
+void node_connection::handle_timeouts() {
+ auto now = std::chrono::steady_clock::now();
+
+ {
+ std::lock_guard lock(m_request_handlers_mutex);
+
+ std::vector<int64_t> keys_for_erasure;
Review Comment:
Let's try avoiding additional memory allocation here. Let's erase values as
we move through the map. We hold the lock anyway. It can be done with iterator:
```cpp
auto it = map.begin();
while (it != map.end()) {
if (something) {
it = map.erase(it);
continue;
}
++it;
}
```
##########
modules/platforms/cpp/tests/fake_server/CMakeLists.txt:
##########
@@ -0,0 +1,30 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+project(ignite-fake-server)
Review Comment:
Why is it in a separate package?
##########
modules/platforms/cpp/ignite/client/detail/node_connection.h:
##########
@@ -119,9 +139,14 @@ class node_connection : public
std::enable_shared_from_this<node_connection> {
buffer.write_length_header();
}
+ std::optional<std::chrono::time_point<std::chrono::steady_clock>>
timeout{};
+ if (m_configuration.get_operation_timeout().count() > 0) {
+ timeout = std::chrono::steady_clock::now() +
m_configuration.get_operation_timeout();
Review Comment:
Why? What exactly should be optimized here?
##########
modules/platforms/cpp/ignite/client/ignite_client_configuration.h:
##########
@@ -187,6 +192,28 @@ class ignite_client_configuration {
m_heartbeat_interval = heartbeat_interval;
}
+ /**
+ * Gets the operation timeout, in milliseconds. Default is 0 (no timeout).
+ *
+ * An "operation" is a single client request to the server. Some public
API calls may involve multiple operations,
+ * in which case the operation timeout is applied to each individual
network call.
+ *
+ * @return Operation timeout, in milliseconds.
+ */
Review Comment:
We can see from the type it's in ms. Also, when std::chrono duration types
are used it's not really important if it in milliseconds or in something else
as this value can be converted freely.
```suggestion
/**
* Gets the operation timeout. Default is 0 (no timeout).
*
* An "operation" is a single client request to the server. Some public
API calls may involve multiple operations,
* in which case the operation timeout is applied to each individual
network call.
*
* @return Operation timeout.
*/
```
##########
modules/platforms/cpp/ignite/client/ignite_client_configuration.h:
##########
@@ -187,6 +192,28 @@ class ignite_client_configuration {
m_heartbeat_interval = heartbeat_interval;
}
+ /**
+ * Gets the operation timeout, in milliseconds. Default is 0 (no timeout).
+ *
+ * An "operation" is a single client request to the server. Some public
API calls may involve multiple operations,
+ * in which case the operation timeout is applied to each individual
network call.
+ *
+ * @return Operation timeout, in milliseconds.
+ */
+ [[nodiscard]] std::chrono::milliseconds get_operation_timeout() const {
return m_operation_timeout; }
+
+ /**
+ * Sets the operation timeout.
+ */
Review Comment:
Add some description of the parameter.
```suggestion
/**
* Sets the operation timeout.
*
* See @ref get_operation_timeout() for details.
*
* @param operation_timeout Operation timeout.
*/
```
##########
modules/platforms/cpp/ignite/client/detail/node_connection.cpp:
##########
@@ -203,7 +208,46 @@ std::shared_ptr<response_handler>
node_connection::find_handler_unsafe(std::int6
if (it == m_request_handlers.end())
return {};
- return it->second;
+ return it->second.handler;
+}
+
+void node_connection::handle_timeouts() {
+ auto now = std::chrono::steady_clock::now();
+
+ {
+ std::lock_guard lock(m_request_handlers_mutex);
+
+ std::vector<int64_t> keys_for_erasure;
+
+ for (auto& [id, req] : m_request_handlers) {
+ if (req.timeouts_at > now) {
Review Comment:
It seems wrong. Do tests pass?
```suggestion
if (req.timeouts_at < now) {
```
##########
modules/platforms/cpp/ignite/client/detail/node_connection.cpp:
##########
@@ -203,7 +208,46 @@ std::shared_ptr<response_handler>
node_connection::find_handler_unsafe(std::int6
if (it == m_request_handlers.end())
return {};
- return it->second;
+ return it->second.handler;
+}
+
+void node_connection::handle_timeouts() {
+ auto now = std::chrono::steady_clock::now();
+
+ {
+ std::lock_guard lock(m_request_handlers_mutex);
+
+ std::vector<int64_t> keys_for_erasure;
+
+ for (auto& [id, req] : m_request_handlers) {
+ if (req.timeouts_at > now) {
+
+ std::stringstream ss;
+ ss << "Operation timeout [req_id=" << id << "]";
+ auto res =
req.handler->set_error(ignite_error(error::code::CLIENT_OPERATION_TIMEOUT,ss.str()));
Review Comment:
I think we should also log it as a warning here.
##########
modules/platforms/cpp/tests/fake_server/tcp_client_channel.cpp:
##########
@@ -0,0 +1,79 @@
+//
+// Created by Ed on 25.11.2025.
+//
Review Comment:
License.
##########
modules/platforms/cpp/tests/fake_server/response_action.cpp:
##########
@@ -0,0 +1,5 @@
+//
+// Created by Ed on 25.11.2025.
+//
+
+#include "response_action.h"
Review Comment:
Empty file? Let's remove.
##########
modules/platforms/cpp/ignite/client/ignite_client_configuration.h:
##########
@@ -47,6 +47,11 @@ class ignite_client_configuration {
*/
static constexpr std::chrono::milliseconds DEFAULT_HEARTBEAT_INTERVAL =
std::chrono::seconds(30);
+ /**
+ * Default operation
+ */
+ static constexpr std::chrono::milliseconds DEFAULT_OPERATION_TIMEOUT =
std::chrono::milliseconds(0);
Review Comment:
```suggestion
static constexpr std::chrono::milliseconds DEFAULT_OPERATION_TIMEOUT{0};
```
##########
modules/platforms/cpp/tests/client-test/main.cpp:
##########
@@ -72,7 +72,7 @@ int main(int argc, char **argv) {
runner.stop();
});
- if (!check_test_node_connectable(std::chrono::seconds(5))) {
+ if (!check_test_node_connectable(std::chrono::seconds(5)) && false) {
Review Comment:
Revert.
##########
modules/platforms/cpp/tests/fake_server/connection_test.cpp:
##########
@@ -0,0 +1,60 @@
+//
+// Created by Ed on 21.11.2025.
+//
Review Comment:
License
##########
modules/platforms/cpp/ignite/client/ignite_client_configuration.h:
##########
@@ -187,6 +192,28 @@ class ignite_client_configuration {
m_heartbeat_interval = heartbeat_interval;
}
+ /**
+ * Gets the operation timeout, in milliseconds. Default is 0 (no timeout).
+ *
+ * An "operation" is a single client request to the server. Some public
API calls may involve multiple operations,
+ * in which case the operation timeout is applied to each individual
network call.
+ *
+ * @return Operation timeout, in milliseconds.
+ */
+ [[nodiscard]] std::chrono::milliseconds get_operation_timeout() const {
return m_operation_timeout; }
+
+ /**
+ * Sets the operation timeout.
+ */
+ void set_operation_timeout(std::chrono::milliseconds operation_timeout) {
+ if (operation_timeout.count() < 0) {
+ throw ignite_error(error::code::ILLEGAL_ARGUMENT, "Operation
timeout can't be negative: "
Review Comment:
Let's add a test for this.
##########
modules/platforms/cpp/ignite/client/ignite_client_configuration.h:
##########
@@ -187,6 +192,28 @@ class ignite_client_configuration {
m_heartbeat_interval = heartbeat_interval;
}
+ /**
+ * Gets the operation timeout, in milliseconds. Default is 0 (no timeout).
+ *
+ * An "operation" is a single client request to the server. Some public
API calls may involve multiple operations,
+ * in which case the operation timeout is applied to each individual
network call.
+ *
+ * @return Operation timeout, in milliseconds.
+ */
Review Comment:
Also, let's clarify the "single request" part. I think we should clarify
that the timeout is for the interval between sending a request and receiving a
response to it. We can also clarify here that it's not applicable for the long
running compute jobs execution, as the response is received as soon as the job
is started by the cluster, and when it's done we receive a notification from
the server which is a different mechanism.
##########
modules/platforms/cpp/tests/fake_server/tcp_client_channel.h:
##########
@@ -0,0 +1,54 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+#include <atomic>
+#include <cstddef>
+#include <ignite/common/ignite_error.h>
+#include <ignite/protocol/utils.h>
+#include <netinet/in.h>
+#include <sys/socket.h>
+#include <unistd.h>
Review Comment:
Is this implementation for Linux only?
##########
modules/platforms/cpp/tests/fake_server/fake_server.cpp:
##########
@@ -0,0 +1,5 @@
+//
+// Created by Ed on 21.11.2025.
+//
+
+#include "fake_server.h"
Review Comment:
Empty file, let's remove.
--
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]
