isapego commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2549643636
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -802,4 +834,34 @@ void sql_connection::on_observable_timestamp(std::int64_t
timestamp) {
}
}
+void sql_connection::send_heartbeat() {
+ catch_errors([&] {
+ sync_request(protocol::client_operation::HEARTBEAT,
[&](protocol::writer &) {});
+ });
+
+ plan_heartbeat(m_heartbeat_interval);
Review Comment:
I don't think it should be here
##########
modules/platforms/cpp/ignite/client/detail/node_connection.cpp:
##########
@@ -170,19 +171,19 @@ ignite_result<void>
node_connection::process_handshake_rsp(bytes_view msg) {
on_observable_timestamp_changed(response.observable_timestamp);
- auto heartbeat_ms = m_configuration.get_heartbeat_interval().count();
- if (heartbeat_ms) {
- assert(heartbeat_ms > 0);
+ auto heartbeat_ms = m_configuration.get_heartbeat_interval();
+ if (heartbeat_ms.count()) {
+ assert(heartbeat_ms.count() > 0);
- heartbeat_ms = std::min(response.idle_timeout_ms / 3, heartbeat_ms);
- heartbeat_ms = std::max(MIN_HEARTBEAT_INTERVAL.count(), heartbeat_ms);
+ heartbeat_ms = min(std::chrono::milliseconds(response.idle_timeout_ms
/ 3), heartbeat_ms);
Review Comment:
What if idle timeout is zero (disabled)?
##########
modules/platforms/cpp/ignite/client/detail/node_connection.cpp:
##########
@@ -170,19 +171,19 @@ ignite_result<void>
node_connection::process_handshake_rsp(bytes_view msg) {
on_observable_timestamp_changed(response.observable_timestamp);
- auto heartbeat_ms = m_configuration.get_heartbeat_interval().count();
- if (heartbeat_ms) {
- assert(heartbeat_ms > 0);
+ auto heartbeat_ms = m_configuration.get_heartbeat_interval();
+ if (heartbeat_ms.count()) {
+ assert(heartbeat_ms.count() > 0);
- heartbeat_ms = std::min(response.idle_timeout_ms / 3, heartbeat_ms);
- heartbeat_ms = std::max(MIN_HEARTBEAT_INTERVAL.count(), heartbeat_ms);
+ heartbeat_ms = min(std::chrono::milliseconds(response.idle_timeout_ms
/ 3), heartbeat_ms);
Review Comment:
By the way, why won't we extract the whole logic somewhere to `protocol`
lib, so we can re-use the same logic in client and ODBC? I mean, it's the same
logic and is quite complex and error-prone to re-write it every time.
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -802,4 +834,34 @@ void sql_connection::on_observable_timestamp(std::int64_t
timestamp) {
}
}
+void sql_connection::send_heartbeat() {
+ catch_errors([&] {
+ sync_request(protocol::client_operation::HEARTBEAT,
[&](protocol::writer &) {});
+ });
+
+ plan_heartbeat(m_heartbeat_interval);
+}
+
+void sql_connection::on_heartbeat_timeout() {
+ auto idle_for = std::chrono::duration_cast<std::chrono::milliseconds>(
+ std::chrono::steady_clock::now() - m_last_message_ts);
+
+ if (idle_for > m_heartbeat_interval) {
+ send_heartbeat();
+ } else {
+ auto sleep_for = m_heartbeat_interval - idle_for;
+ plan_heartbeat(sleep_for);
+ }
Review Comment:
Should not it be
```suggestion
auto sleep_for = m_heartbeat_interval;
if (idle_for > m_heartbeat_interval) {
send_heartbeat();
} else {
sleep_for = m_heartbeat_interval - idle_for;
}
plan_heartbeat(sleep_for);
```
##########
modules/platforms/cpp/ignite/odbc/config/configuration.h:
##########
@@ -57,6 +57,9 @@ class configuration {
/** Default value for SSL mode. */
static inline const ssl_mode_t ssl_mode{ssl_mode_t::DISABLE};
+
+ /** Default heartbeat interval. */
+ static inline const std::chrono::seconds
heartbeat_interval{std::chrono::seconds{30}};
Review Comment:
```suggestion
static inline const std::chrono::seconds heartbeat_interval{30};
```
--
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]
