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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
