IGNITE-27056 added request timeouts to C++ client #7080

Erixonich · 2025-11-25T20:27:43Z

https://issues.apache.org/jira/browse/IGNITE-27056

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

# Conflicts: # modules/platforms/cpp/ignite/client/detail/node_connection.h

Erixonich · 2025-11-26T16:49:44Z

modules/platforms/cpp/ignite/client/detail/node_connection.h


+        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();


In java client there is an optimization for calling analog of ::now(). I am not aware of performance of this method. Should we research if similiar optimisation is required?

Why? What exactly should be optimized here?

TLDR:
I was under impression that java's System.currentTimeMillis() and steady_clock::now() have similar implementation and drawbacks. But learning a bit deeper revealed to me that system_clock::now() should be analogous to System.currentTimeMillis().

as I said I don't know any performance issues of this method. In java case there were some issues with analogous System.currentTimeMillis() on windows platform. That could be a reason why in ignite codebase we have org.apache.ignite.internal.util.FastTimestamps#coarseCurrentTimeMillis.

what optimization could be (if calling std::chrono::steady_clock::now() would be slower than we can afford). We spawn a thread which updates atomic value every X seconds. Instead of calling ::now() we read from atomic variable. As I googled a bit calling steady_clock::now() doesn't switch context from userspace to kernelspace (on opposing of calling system_clock::now()).

I am pretty sure that we don't want to do it right now, but maybe small task to research performance impact of calling ::now on each request.

Erixonich · 2025-11-26T16:52:23Z

modules/platforms/cpp/tests/compatibility-tests/main.cpp

 using namespace ignite;

-const std::vector<std::string> DEFAULT_VERSIONS = {"9.1.0", "9.1.1", "9.1.2", "9.1.3", "9.1.4"};
+const std::vector<std::string> DEFAULT_VERSIONS = {"3.0.0"};


unrelated to this ticket but mistake I made in previous PR.

What about 3.1.0?

Copilot

Pull request overview

This PR adds request timeout support to the C++ client for Apache Ignite. The implementation allows clients to specify an operation timeout that applies to individual network operations, causing them to fail if the server doesn't respond within the specified time.

Key changes:

Adds operation_timeout configuration parameter to ignite_client_configuration
Implements timeout tracking in node_connection using pending request metadata
Creates a fake server test infrastructure to simulate timeout scenarios
Adds new error code CLIENT_OPERATION_TIMEOUT for timeout failures

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
modules/platforms/cpp/ignite/client/ignite_client_configuration.h	Adds operation timeout configuration getter/setter with validation
modules/platforms/cpp/ignite/common/error_codes.h	Adds CLIENT_OPERATION_TIMEOUT error code
modules/api/src/main/java/org/apache/ignite/lang/ErrorGroups.java	Adds corresponding Java error code
modules/platforms/cpp/ignite/client/detail/node_connection.h	Adds pending_request structure with timeout metadata
modules/platforms/cpp/ignite/client/detail/node_connection.cpp	Implements timeout handling logic with periodic checks
modules/platforms/cpp/tests/fake_server/*	New fake server infrastructure for testing timeout scenarios
modules/platforms/cpp/tests/fake_server/connection_test.cpp	Tests for handshake and timeout functionality
modules/platforms/cpp/ignite/protocol/buffer_adapter.h	Refactors magic number to use constant
modules/platforms/cpp/tests/client-test/main.cpp	Temporary bypass of connection check (likely for testing)
modules/platforms/cpp/tests/compatibility-tests/main.cpp	Updates default version for compatibility testing

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

modules/platforms/cpp/ignite/client/ignite_client_configuration.h

Copilot · 2025-11-26T23:57:14Z

modules/platforms/cpp/ignite/client/detail/node_connection.cpp

+        std::vector<int64_t> keys_for_erasure;
+
+        for (auto& [id, req] : m_request_handlers) {
+            if (req.timeouts_at > now) {


The comparison logic is inverted. This should be req.timeouts_at < now to check if the request has timed out (i.e., the timeout time point is in the past). Currently, it marks requests as timed out when they haven't expired yet.

Suggested change

if (req.timeouts_at > now) {

if (req.timeouts_at < now) {

modules/platforms/cpp/tests/fake_server/fake_server.h

modules/platforms/cpp/tests/fake_server/response_action.h

modules/platforms/cpp/tests/fake_server/tcp_client_channel.h

modules/platforms/cpp/tests/fake_server/response_action.h

Copilot · 2025-11-26T23:57:15Z

modules/platforms/cpp/ignite/client/detail/node_connection.cpp

+
+                std::stringstream ss;
+                ss << "Operation timeout [req_id=" << id << "]";
+                auto res = req.handler->set_error(ignite_error(error::code::CLIENT_OPERATION_TIMEOUT,ss.str()));


Missing space after comma in function call. Should be ss.str()) → ss.str()) with proper spacing: CLIENT_OPERATION_TIMEOUT, ss.str().

Suggested change

auto res = req.handler->set_error(ignite_error(error::code::CLIENT_OPERATION_TIMEOUT,ss.str()));

auto res = req.handler->set_error(ignite_error(error::code::CLIENT_OPERATION_TIMEOUT, ss.str()));

isapego · 2025-11-26T23:59:52Z

modules/platforms/cpp/ignite/client/detail/node_connection.h


+        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();


Why? What exactly should be optimized here?

isapego · 2025-11-27T00:03:10Z

modules/platforms/cpp/ignite/client/detail/node_connection.cpp

+                std::stringstream ss;
+                ss << "Operation timeout [req_id=" << id << "]";
+                auto res = req.handler->set_error(ignite_error(error::code::CLIENT_OPERATION_TIMEOUT,ss.str()));


I think we should also log it as a warning here.

isapego · 2025-11-27T00:04:08Z

modules/platforms/cpp/ignite/client/detail/node_connection.cpp

+        std::vector<int64_t> keys_for_erasure;
+
+        for (auto& [id, req] : m_request_handlers) {
+            if (req.timeouts_at > now) {


It seems wrong. Do tests pass?

Suggested change

if (req.timeouts_at > now) {

if (req.timeouts_at < now) {

I will add a test which would fail with suc incorrect code. Thats left as artifact of refactoring.

isapego · 2025-11-27T00:11:47Z

modules/platforms/cpp/ignite/client/detail/node_connection.cpp

+    {
+        std::lock_guard lock(m_request_handlers_mutex);
+
+        std::vector<int64_t> keys_for_erasure;


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:

auto it = map.begin(); while (it != map.end()) { if (something) { it = map.erase(it); continue; } ++it; }

isapego · 2025-11-27T00:16:19Z

modules/platforms/cpp/ignite/client/ignite_client_configuration.h

+    /**
+     * Default operation
+     */
+    static constexpr std::chrono::milliseconds DEFAULT_OPERATION_TIMEOUT = std::chrono::milliseconds(0);


Suggested change

static constexpr std::chrono::milliseconds DEFAULT_OPERATION_TIMEOUT = std::chrono::milliseconds(0);

static constexpr std::chrono::milliseconds DEFAULT_OPERATION_TIMEOUT{0};

isapego · 2025-11-27T00:36:12Z

modules/platforms/cpp/tests/fake_server/connection_test.cpp

+//
+// Created by Ed on 21.11.2025.
+//


isapego · 2025-11-27T00:38:28Z

modules/platforms/cpp/tests/fake_server/response_action.cpp

+//
+// Created by Ed on 25.11.2025.
+//
+
+#include "response_action.h"


Empty file? Let's remove.

isapego · 2025-11-27T00:38:51Z

modules/platforms/cpp/tests/fake_server/fake_server.cpp

+//
+// Created by Ed on 21.11.2025.
+//
+
+#include "fake_server.h"


Empty file, let's remove.

isapego · 2025-11-27T00:39:34Z

modules/platforms/cpp/tests/fake_server/tcp_client_channel.cpp

+//
+// Created by Ed on 25.11.2025.
+//


isapego · 2025-11-27T00:41:03Z

modules/platforms/cpp/tests/fake_server/tcp_client_channel.h

+#include <netinet/in.h>
+#include <sys/socket.h>
+#include <unistd.h>


Is this implementation for Linux only?

Erixonich added 6 commits November 14, 2025 17:30

wip

7171d63

wip

86f96ba

wip

ab4a207

wip

a28548f

wip

2cf4277

IGNITE-27056 added working timeout test.

5c7d71a

Erixonich requested a review from isapego as a code owner November 25, 2025 20:27

Merge branch 'main' into ignite-27056

859ac61

# Conflicts: # modules/platforms/cpp/ignite/client/detail/node_connection.h

Erixonich commented Nov 26, 2025

View reviewed changes

Erixonich added 3 commits November 26, 2025 22:38

IGNITE-27056 fixed cosmetic issues. Removed unused code.

e5dec2e

IGNITE-27056 fixed error code name

02cd62d

IGNITE-27056 fixed error code name

b333e6e

isapego requested a review from Copilot November 26, 2025 23:46

Copilot started reviewing on behalf of isapego November 26, 2025 23:47 View session

Copilot finished reviewing on behalf of isapego November 26, 2025 23:48

Copilot AI reviewed Nov 26, 2025

View reviewed changes

isapego requested changes Nov 27, 2025

View reviewed changes

	auto res = req.handler->set_error(ignite_error(error::code::CLIENT_OPERATION_TIMEOUT,ss.str()));
	auto res = req.handler->set_error(ignite_error(error::code::CLIENT_OPERATION_TIMEOUT, ss.str()));

	static constexpr std::chrono::milliseconds DEFAULT_OPERATION_TIMEOUT = std::chrono::milliseconds(0);
	static constexpr std::chrono::milliseconds DEFAULT_OPERATION_TIMEOUT{0};

IGNITE-27056 added request timeouts to C++ client #7080

Are you sure you want to change the base?

IGNITE-27056 added request timeouts to C++ client #7080

Uh oh!

Conversation

Erixonich commented Nov 25, 2025

The Review Checklist

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants