You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "arcolight (via GitHub)" <gi...@apache.org> on 2025/11/18 11:27:43 UTC
[PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
arcolight opened a new pull request, #7001:
URL: https://github.com/apache/ignite-3/pull/7001
https://issues.apache.org/jira/browse/IGNITE-27013
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](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2549700322
##########
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:
Yeah, moved to the `on_heartbeat_timeout`
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "isapego (via GitHub)" <gi...@apache.org>.
isapego commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2538520454
##########
modules/platforms/cpp/ignite/odbc/config/configuration.cpp:
##########
@@ -125,6 +128,16 @@ void configuration::from_config_map(const config_map &config_params) {
try_get_string_param(m_ssl_key_file, config_params, key::ssl_key_file);
try_get_string_param(m_ssl_cert_file, config_params, key::ssl_cert_file);
try_get_string_param(m_ssl_ca_file, config_params, key::ssl_ca_file);
+
+ auto heartbeat_interval_it = config_params.find(key::heartbeat_interval);
+ if (heartbeat_interval_it != config_params.end()) {
+ auto heartbeat_interval_opt = parse_int<std::int32_t>(heartbeat_interval_it->second);
+ if (!heartbeat_interval_opt)
+ throw odbc_error(sql_state::S01S00_INVALID_CONNECTION_STRING_ATTRIBUTE,
+ "Invalid page size value: " + heartbeat_interval_it->second);
+
+ m_heartbeat_interval = {std::chrono::milliseconds{*heartbeat_interval_opt}, true};
+ }
Review Comment:
Let's extract is to a separate function like `try_get_string_param()`. Also, the error message is wrong. Let's add a test for invalid configuration parameter checking the message.
##########
modules/platforms/cpp/ignite/odbc/odbc.cpp:
##########
@@ -83,15 +83,17 @@ const char *attr_id_to_string(uint16_t id) {
namespace ignite {
+using sql_connection_ptr = sql_environment::sql_connection_ptr;
Review Comment:
Let's add a comment here explaining why do we use `shared_ptr`
##########
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::milliseconds heartbeat_interval{std::chrono::milliseconds{2000}};
Review Comment:
Why milliseconds, if it's in seconds? Also, the default should be 30 secs as in other clients (please check, I'm not sure about the number, but it's definitely not 2 secs).
```suggestion
static inline const std::chrono::milliseconds heartbeat_interval{std::chrono::seconds{30}};
```
##########
modules/platforms/cpp/ignite/odbc/config/configuration.cpp:
##########
@@ -125,6 +128,16 @@ void configuration::from_config_map(const config_map &config_params) {
try_get_string_param(m_ssl_key_file, config_params, key::ssl_key_file);
try_get_string_param(m_ssl_cert_file, config_params, key::ssl_cert_file);
try_get_string_param(m_ssl_ca_file, config_params, key::ssl_ca_file);
+
+ auto heartbeat_interval_it = config_params.find(key::heartbeat_interval);
+ if (heartbeat_interval_it != config_params.end()) {
+ auto heartbeat_interval_opt = parse_int<std::int32_t>(heartbeat_interval_it->second);
+ if (!heartbeat_interval_opt)
+ throw odbc_error(sql_state::S01S00_INVALID_CONNECTION_STRING_ATTRIBUTE,
+ "Invalid page size value: " + heartbeat_interval_it->second);
+
+ m_heartbeat_interval = {std::chrono::milliseconds{*heartbeat_interval_opt}, true};
+ }
Review Comment:
Also, check here that interval is >= 0, because we have `assert` later assuming it should never be negative.
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -178,6 +182,19 @@ sql_result sql_connection::internal_establish(const configuration &cfg) {
bool errors = get_diagnostic_records().get_status_records_number() > 0;
+ auto heartbeat_ms = m_config.get_heartbeat_interval().get_value().count();
+
+ if (heartbeat_ms) {
+ assert(heartbeat_ms > 0);
+
+ heartbeat_ms = std::max(MIN_HEARTBEAT_INTERVAL.count(), heartbeat_ms);
+ }
+ m_heartbeat_interval = std::chrono::milliseconds(heartbeat_ms);
Review Comment:
We should also use `idle_timeout` from handshake response to make sure heartbeat interval is never below idle timeout. See how it's done in C++ Client.
##########
modules/platforms/cpp/ignite/odbc/sql_environment.cpp:
##########
@@ -20,8 +20,15 @@
namespace ignite {
-sql_connection *sql_environment::create_connection() {
- sql_connection *connection;
+sql_environment::sql_environment()
+ : m_timer_thread(detail::thread_timer::start([&] (auto&& err) {
+ std::cout << "[SQL ENV CTOR] error: " << err.what() << std::endl;
Review Comment:
Use logger for logs like these. But it's probably is not needed - there is logging enabled when a new status record is added.
##########
modules/platforms/cpp/tests/odbc-test/odbc_suite.h:
##########
@@ -54,6 +54,16 @@ class odbc_suite : public virtual ::testing::Test, public odbc_connection {
return res;
}
+ /**
+ * Get heartbeat interval for tests.
+ *
+ * @return Heartbeat interval.
+ */
+ static std::chrono::milliseconds get_heartbeat_interval() {
+ using namespace std::chrono_literals;
+ return 2000ms;
Review Comment:
Is not it's easier to read?
```suggestion
return 2s;
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2538618074
##########
modules/platforms/cpp/ignite/odbc/sql_environment.cpp:
##########
@@ -20,8 +20,15 @@
namespace ignite {
-sql_connection *sql_environment::create_connection() {
- sql_connection *connection;
+sql_environment::sql_environment()
+ : m_timer_thread(detail::thread_timer::start([&] (auto&& err) {
+ std::cout << "[SQL ENV CTOR] error: " << err.what() << std::endl;
Review Comment:
Yeah, I already fixed that. Missed this debug output.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2549466994
##########
modules/platforms/cpp/ignite/odbc/config/configuration.cpp:
##########
@@ -125,6 +128,16 @@ void configuration::from_config_map(const config_map &config_params) {
try_get_string_param(m_ssl_key_file, config_params, key::ssl_key_file);
try_get_string_param(m_ssl_cert_file, config_params, key::ssl_cert_file);
try_get_string_param(m_ssl_ca_file, config_params, key::ssl_ca_file);
+
+ auto heartbeat_interval_it = config_params.find(key::heartbeat_interval);
+ if (heartbeat_interval_it != config_params.end()) {
+ auto heartbeat_interval_opt = parse_int<std::int32_t>(heartbeat_interval_it->second);
+ if (!heartbeat_interval_opt)
+ throw odbc_error(sql_state::S01S00_INVALID_CONNECTION_STRING_ATTRIBUTE,
+ "Invalid page size value: " + heartbeat_interval_it->second);
+
+ m_heartbeat_interval = {std::chrono::milliseconds{*heartbeat_interval_opt}, true};
+ }
Review Comment:
Agreed, fixed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "isapego (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2549662383
##########
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:
If it is zero, we will set timeout to the `MIN_HEARTBEAT_INTERVAL` on the next line.
Yes, I can extract this.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2549467725
##########
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::milliseconds heartbeat_interval{std::chrono::milliseconds{2000}};
Review Comment:
Right, I used interval from tests. Fixed.
##########
modules/platforms/cpp/ignite/odbc/odbc.cpp:
##########
@@ -83,15 +83,17 @@ const char *attr_id_to_string(uint16_t id) {
namespace ignite {
+using sql_connection_ptr = sql_environment::sql_connection_ptr;
Review Comment:
Fixed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2538499107
##########
modules/platforms/cpp/ignite/odbc/config/configuration.cpp:
##########
@@ -125,6 +128,16 @@ void configuration::from_config_map(const config_map &config_params) {
try_get_string_param(m_ssl_key_file, config_params, key::ssl_key_file);
try_get_string_param(m_ssl_cert_file, config_params, key::ssl_cert_file);
try_get_string_param(m_ssl_ca_file, config_params, key::ssl_ca_file);
+
+ auto heartbeat_interval_it = config_params.find(key::heartbeat_interval);
+ if (heartbeat_interval_it != config_params.end()) {
+ auto heartbeat_interval_opt = parse_int<std::int32_t>(heartbeat_interval_it->second);
+ if (!heartbeat_interval_opt)
+ throw odbc_error(sql_state::S01S00_INVALID_CONNECTION_STRING_ATTRIBUTE,
+ "Invalid page size value: " + heartbeat_interval_it->second);
Review Comment:
Incorrect error message. The error message "Invalid page size value:" should be "Invalid heartbeat interval value:" to correctly describe what is being validated.
```suggestion
"Invalid heartbeat interval value: " + heartbeat_interval_it->second);
```
##########
modules/platforms/cpp/ignite/odbc/sql_environment.cpp:
##########
@@ -32,16 +39,24 @@ void sql_environment::deregister_connection(sql_connection *conn) {
m_connections.erase(conn);
}
-sql_result sql_environment::internal_create_connection(sql_connection *&conn) {
- conn = new sql_connection(this);
+sql_result sql_environment::internal_create_connection(sql_connection_ptr *&conn) {
+ auto conn_raw = new(std::nothrow) sql_connection(this, m_timer_thread);
+ if (!conn_raw) {
+ add_status_record(sql_state::SHY001_MEMORY_ALLOCATION, "Not enough memory.");
+ return sql_result::AI_ERROR;
+ }
+
+ conn = new(std::nothrow) sql_connection_ptr(conn_raw);
if (!conn) {
+ delete conn;
Review Comment:
Memory leak: When the second allocation fails, the code attempts to `delete conn;` but `conn` is the pointer that just failed to allocate and is likely null or invalid. Instead, `conn_raw` should be deleted here to avoid leaking the first allocation.
```suggestion
delete conn_raw;
```
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -802,4 +822,38 @@ void sql_connection::on_observable_timestamp(std::int64_t timestamp) {
}
}
+void sql_connection::send_heartbeat() {
+ network::data_buffer_owning response;
+ auto success = catch_errors([&] {
+ response = sync_request(protocol::client_operation::HEARTBEAT, [&](protocol::writer &) {});
+ });
+
+ UNUSED_VALUE(response);
+
+ if (success)
+ plan_heartbeat(m_heartbeat_interval);
Review Comment:
Potential issue with heartbeat rescheduling. If `send_heartbeat()` fails (when `catch_errors` returns false), the heartbeat is not rescheduled, which means heartbeats will stop entirely. Consider rescheduling the heartbeat even on failure, potentially with a backoff strategy, to ensure the connection remains monitored.
```suggestion
// Always reschedule the heartbeat, even if sending failed.
plan_heartbeat(m_heartbeat_interval);
```
##########
modules/platforms/cpp/ignite/odbc/sql_connection.h:
##########
@@ -40,9 +41,12 @@ class sql_statement;
/**
* ODBC node connection.
*/
-class sql_connection : public diagnosable_adapter {
+class sql_connection : public diagnosable_adapter, public std::enable_shared_from_this<sql_connection> {
Review Comment:
Extra whitespace in the `enable_shared_from_this` declaration. There's a double space before the opening brace: `enable_shared_from_this<sql_connection> {` should be `enable_shared_from_this<sql_connection> {`.
```suggestion
class sql_connection : public diagnosable_adapter, public std::enable_shared_from_this<sql_connection> {
```
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -70,6 +70,10 @@ std::vector<ignite::end_point> collect_addresses(const ignite::configuration &cf
namespace ignite {
+sql_connection::~sql_connection() {
Review Comment:
Potential race condition in destructor. The destructor calls `deregister()` which modifies the environment's connection set. If heartbeat callbacks are still scheduled or executing when the destructor runs, they could attempt to access the connection after it's been deregistered or partially destroyed. Consider canceling any pending heartbeat timers in the destructor before calling `deregister()`.
```suggestion
void sql_connection::cancel_heartbeat() {
// TODO: Implement heartbeat timer cancellation logic here.
}
sql_connection::~sql_connection() {
cancel_heartbeat();
cancel_heartbeat();
```
##########
modules/platforms/cpp/ignite/odbc/sql_environment.cpp:
##########
@@ -20,8 +20,15 @@
namespace ignite {
-sql_connection *sql_environment::create_connection() {
- sql_connection *connection;
+sql_environment::sql_environment()
+ : m_timer_thread(detail::thread_timer::start([&] (auto&& err) {
+ std::cout << "[SQL ENV CTOR] error: " << err.what() << std::endl;
Review Comment:
Debug output left in production code. The line `std::cout << "[SQL ENV CTOR] error: " << err.what() << std::endl;` should be removed or replaced with proper logging.
```suggestion
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2538540938
##########
modules/platforms/cpp/ignite/odbc/sql_environment.cpp:
##########
@@ -20,8 +20,15 @@
namespace ignite {
-sql_connection *sql_environment::create_connection() {
- sql_connection *connection;
+sql_environment::sql_environment()
+ : m_timer_thread(detail::thread_timer::start([&] (auto&& err) {
+ std::cout << "[SQL ENV CTOR] error: " << err.what() << std::endl;
Review Comment:
Fixed
##########
modules/platforms/cpp/ignite/odbc/sql_environment.cpp:
##########
@@ -32,16 +39,24 @@ void sql_environment::deregister_connection(sql_connection *conn) {
m_connections.erase(conn);
}
-sql_result sql_environment::internal_create_connection(sql_connection *&conn) {
- conn = new sql_connection(this);
+sql_result sql_environment::internal_create_connection(sql_connection_ptr *&conn) {
+ auto conn_raw = new(std::nothrow) sql_connection(this, m_timer_thread);
+ if (!conn_raw) {
+ add_status_record(sql_state::SHY001_MEMORY_ALLOCATION, "Not enough memory.");
+ return sql_result::AI_ERROR;
+ }
+
+ conn = new(std::nothrow) sql_connection_ptr(conn_raw);
if (!conn) {
+ delete conn;
Review Comment:
Fixed
##########
modules/platforms/cpp/ignite/odbc/sql_connection.h:
##########
@@ -40,9 +41,12 @@ class sql_statement;
/**
* ODBC node connection.
*/
-class sql_connection : public diagnosable_adapter {
+class sql_connection : public diagnosable_adapter, public std::enable_shared_from_this<sql_connection> {
Review Comment:
Fixed
##########
modules/platforms/cpp/ignite/odbc/config/configuration.cpp:
##########
@@ -125,6 +128,16 @@ void configuration::from_config_map(const config_map &config_params) {
try_get_string_param(m_ssl_key_file, config_params, key::ssl_key_file);
try_get_string_param(m_ssl_cert_file, config_params, key::ssl_cert_file);
try_get_string_param(m_ssl_ca_file, config_params, key::ssl_ca_file);
+
+ auto heartbeat_interval_it = config_params.find(key::heartbeat_interval);
+ if (heartbeat_interval_it != config_params.end()) {
+ auto heartbeat_interval_opt = parse_int<std::int32_t>(heartbeat_interval_it->second);
+ if (!heartbeat_interval_opt)
+ throw odbc_error(sql_state::S01S00_INVALID_CONNECTION_STRING_ATTRIBUTE,
+ "Invalid page size value: " + heartbeat_interval_it->second);
Review Comment:
Fixed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "isapego (via GitHub)" <gi...@apache.org>.
isapego merged PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2549664326
##########
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:
Yes, right.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2549468458
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -178,6 +182,19 @@ sql_result sql_connection::internal_establish(const configuration &cfg) {
bool errors = get_diagnostic_records().get_status_records_number() > 0;
+ auto heartbeat_ms = m_config.get_heartbeat_interval().get_value().count();
+
+ if (heartbeat_ms) {
+ assert(heartbeat_ms > 0);
+
+ heartbeat_ms = std::max(MIN_HEARTBEAT_INTERVAL.count(), heartbeat_ms);
+ }
+ m_heartbeat_interval = std::chrono::milliseconds(heartbeat_ms);
Review Comment:
Fixed.
##########
modules/platforms/cpp/tests/odbc-test/odbc_suite.h:
##########
@@ -54,6 +54,16 @@ class odbc_suite : public virtual ::testing::Test, public odbc_connection {
return res;
}
+ /**
+ * Get heartbeat interval for tests.
+ *
+ * @return Heartbeat interval.
+ */
+ static std::chrono::milliseconds get_heartbeat_interval() {
+ using namespace std::chrono_literals;
+ return 2000ms;
Review Comment:
Right, it is simpler, of course.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2549699494
##########
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:
Ok, right, makes sense.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2538575159
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -70,6 +70,10 @@ std::vector<ignite::end_point> collect_addresses(const ignite::configuration &cf
namespace ignite {
+sql_connection::~sql_connection() {
Review Comment:
Environment holds only raw pointer to the `shared_pointer`, so deregistration does not mean we delete connection, it will be done by separate call and we use `weak_ptr` inside the closure in the `plan_heartbeat()`, so `lock()` will fail and nothing will be called in case connection is already destroyed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27013 ODBC 3.0: Implement heartbeats [ignite-3]
Posted by "arcolight (via GitHub)" <gi...@apache.org>.
arcolight commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2538561901
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -802,4 +822,38 @@ void sql_connection::on_observable_timestamp(std::int64_t timestamp) {
}
}
+void sql_connection::send_heartbeat() {
+ network::data_buffer_owning response;
+ auto success = catch_errors([&] {
+ response = sync_request(protocol::client_operation::HEARTBEAT, [&](protocol::writer &) {});
+ });
+
+ UNUSED_VALUE(response);
+
+ if (success)
+ plan_heartbeat(m_heartbeat_interval);
Review Comment:
Right, fixed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org