Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix monitor timeout #149

Merged
merged 3 commits into from
May 11, 2023
Merged

Fix monitor timeout #149

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77aea6c
fix monitor timeout
yanw-bq May 9, 2023
a8e2e83
fix login timeout and unit tests
yanw-bq May 10, 2023
f10ac52
pass monitor flag
yanw-bq May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions driver/connect.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ class dbc_guard
@return Standard SQLRETURN code. If it is @c SQL_SUCCESS or @c
SQL_SUCCESS_WITH_INFO, a connection has been established.
*/
SQLRETURN DBC::connect(DataSource *dsrc, bool failover_enabled)
SQLRETURN DBC::connect(DataSource *dsrc, bool failover_enabled, bool is_monitor_connection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about defining an Enum for the three different possibilities instead of using two boolean flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_monitor_connection is optional, we only use it in monitor class. No need to change other places when we call DBC::connect()(except unit tests, need to work around google tests)
Also, there are places in the code where we only care about failover_enabled, but not is_monitor_connection, like FAILOVER_HANDLER::reconnect(bool failover_enabled), I don't think enum really fits here.

{
SQLRETURN rc = SQL_SUCCESS;
unsigned long flags;
Expand Down Expand Up @@ -385,7 +385,12 @@ SQLRETURN DBC::connect(DataSource *dsrc, bool failover_enabled)
read_timeout = get_network_timeout(dsrc->network_timeout);
write_timeout = read_timeout;
}
else
else if (is_monitor_connection)
{
connect_timeout = get_network_timeout(dsrc->read_timeout);
read_timeout = get_network_timeout(dsrc->read_timeout);
write_timeout = get_network_timeout(dsrc->write_timeout);
} else
{
connect_timeout = get_connect_timeout(login_timeout);
read_timeout = get_network_timeout(dsrc->read_timeout);
Expand Down
6 changes: 3 additions & 3 deletions driver/connection_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ CONNECTION_HANDLER::CONNECTION_HANDLER(DBC* dbc) : dbc{dbc} {}

CONNECTION_HANDLER::~CONNECTION_HANDLER() = default;

SQLRETURN CONNECTION_HANDLER::do_connect(DBC* dbc_ptr, DataSource* ds, bool failover_enabled) {
SQLRETURN CONNECTION_HANDLER::do_connect(DBC* dbc_ptr, DataSource* ds, bool failover_enabled, bool is_monitor_connection) {
return dbc_ptr->connect(ds, failover_enabled);
}

CONNECTION_PROXY* CONNECTION_HANDLER::connect(std::shared_ptr<HOST_INFO> host_info, DataSource* ds) {
CONNECTION_PROXY* CONNECTION_HANDLER::connect(std::shared_ptr<HOST_INFO> host_info, DataSource* ds, bool is_monitor_connection) {

if (dbc == nullptr || host_info == nullptr) {
return nullptr;
Expand All @@ -76,7 +76,7 @@ CONNECTION_PROXY* CONNECTION_HANDLER::connect(std::shared_ptr<HOST_INFO> host_in

CONNECTION_PROXY* new_connection = nullptr;
CLEAR_DBC_ERROR(dbc_clone);
const SQLRETURN rc = do_connect(dbc_clone, ds_to_use, ds_to_use->enable_cluster_failover);
const SQLRETURN rc = do_connect(dbc_clone, ds_to_use, ds_to_use->enable_cluster_failover, is_monitor_connection);

if (rc == SQL_SUCCESS || rc == SQL_SUCCESS_WITH_INFO) {
new_connection = dbc_clone->connection_proxy;
Expand Down
4 changes: 2 additions & 2 deletions driver/connection_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ class CONNECTION_HANDLER {
CONNECTION_HANDLER(DBC* dbc);
virtual ~CONNECTION_HANDLER();

virtual SQLRETURN do_connect(DBC* dbc_ptr, DataSource* ds, bool failover_enabled);
virtual CONNECTION_PROXY* connect(std::shared_ptr<HOST_INFO> host_info, DataSource* ds);
virtual SQLRETURN do_connect(DBC* dbc_ptr, DataSource* ds, bool failover_enabled, bool is_monitor_connection = false);
virtual CONNECTION_PROXY* connect(std::shared_ptr<HOST_INFO> host_info, DataSource* ds, bool is_monitor_connection = false);
void update_connection(CONNECTION_PROXY* new_connection, const std::string& new_host_name);

private:
Expand Down
2 changes: 1 addition & 1 deletion driver/driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ struct DBC
void remove_desc(DESC *desc);
SQLRETURN set_error(char *state, const char *message, uint errcode);
SQLRETURN set_error(char *state);
SQLRETURN connect(DataSource *dsrc, bool failover_enabled);
SQLRETURN connect(DataSource *dsrc, bool failover_enabled, bool is_monitor_connection = false);
void execute_prep_stmt(MYSQL_STMT *pstmt, std::string &query,
MYSQL_BIND *param_bind, MYSQL_BIND *result_bind);
void init_proxy_chain(DataSource *dsrc);
Expand Down
4 changes: 2 additions & 2 deletions driver/monitor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,12 +226,12 @@ bool MONITOR::connect() {
} else {
// cannot change login_timeout here because no access to dbc
this->ds->read_timeout = timeout_sec;
this->ds->write_timeout = timeout_sec;
}

this->ds->enable_cluster_failover = false;
this->ds->enable_failure_detection= false;

this->connection_proxy = this->connection_handler->connect(this->host, this->ds);
this->connection_proxy = this->connection_handler->connect(this->host, this->ds, true);
if (!this->connection_proxy) {
return false;
}
Expand Down
44 changes: 22 additions & 22 deletions unit_testing/failover_handler_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,9 @@ TEST_F(FailoverHandlerTest, CustomDomain) {
.Times(AtLeast(1));
EXPECT_CALL(*mock_ts, set_cluster_id(_)).Times(0);

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));
EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, true))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, true, false))
.WillOnce(Return(SQL_SUCCESS));

FAILOVER_HANDLER failover_handler(dbc, ds, mock_connection_handler, mock_ts, mock_metrics);
Expand All @@ -132,7 +132,7 @@ TEST_F(FailoverHandlerTest, CustomDomain) {
TEST_F(FailoverHandlerTest, FailoverDisabled) {
ds->enable_cluster_failover = false;

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false)).Times(1);
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false)).Times(1);

FAILOVER_HANDLER failover_handler(dbc, ds, mock_connection_handler, mock_ts, mock_metrics);
failover_handler.init_connection();
Expand All @@ -145,7 +145,7 @@ TEST_F(FailoverHandlerTest, IP_TopologyAvailable_PatternRequired) {

ds_setattr_from_utf8(&ds->server, server);

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_ts, get_topology(_, false)).WillOnce(Return(topology));
Expand All @@ -159,7 +159,7 @@ TEST_F(FailoverHandlerTest, IP_TopologyNotAvailable) {

ds_setattr_from_utf8(&ds->server, server);

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_ts, get_topology(_, false))
Expand All @@ -181,9 +181,9 @@ TEST_F(FailoverHandlerTest, IP_Cluster) {
ds_setattr_from_utf8(&ds->server, server);
ds_setattr_from_utf8(&ds->host_pattern, host_pattern);

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));
EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, true))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, true, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_ts, get_topology(_, false)).WillOnce(Return(topology));
Expand All @@ -208,9 +208,9 @@ TEST_F(FailoverHandlerTest, IP_Cluster_ClusterID) {
ds_setattr_from_utf8(&ds->host_pattern, host_pattern);
ds_setattr_from_utf8(&ds->cluster_id, cluster_id);

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));
EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, true))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, true, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_ts, get_topology(_, false)).WillOnce(Return(topology));
Expand All @@ -232,9 +232,9 @@ TEST_F(FailoverHandlerTest, RDS_Cluster) {
ds_setattr_from_utf8(&ds->server, server);
ds->port = 1234;

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));
EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, true))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, true, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_ts, get_topology(_, false)).WillOnce(Return(topology));
Expand All @@ -255,9 +255,9 @@ TEST_F(FailoverHandlerTest, RDS_CustomCluster) {

ds_setattr_from_utf8(&ds->server, server);

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));
EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, true))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, true, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_ts, get_topology(_, false)).WillOnce(Return(topology));
Expand All @@ -278,9 +278,9 @@ TEST_F(FailoverHandlerTest, RDS_Instance) {

ds_setattr_from_utf8(&ds->server, server);

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));
EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, true))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, true, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_ts, get_topology(_, false)).WillOnce(Return(topology));
Expand All @@ -302,7 +302,7 @@ TEST_F(FailoverHandlerTest, RDS_Proxy) {
ds_setattr_from_utf8(&ds->server, server);
ds->port = 1234;

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_ts, get_topology(_, false)).WillOnce(Return(topology));
Expand All @@ -324,9 +324,9 @@ TEST_F(FailoverHandlerTest, RDS_ReaderCluster) {
ds_setattr_from_utf8(&ds->server, server);
ds->port = 1234;

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));
EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, true))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, true, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_ts, get_topology(_, false)).WillOnce(Return(topology));
Expand All @@ -352,7 +352,7 @@ TEST_F(FailoverHandlerTest, RDS_MultiWriterCluster) {
ds->port = 1234;
ds->enable_cluster_failover = true;

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false)).WillOnce(Return(SQL_SUCCESS));
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false)).WillOnce(Return(SQL_SUCCESS));

auto multi_writer_topology = std::make_shared<CLUSTER_TOPOLOGY_INFO>();
multi_writer_topology->add_host(writer_host);
Expand Down Expand Up @@ -389,10 +389,10 @@ TEST_F(FailoverHandlerTest, ReconnectWithFailoverSettings) {
EXPECT_CALL(*mock_ts, set_cluster_instance_template(_))
.Times(AtLeast(1));

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, false))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, false, false))
.WillOnce(Return(SQL_SUCCESS));

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, true))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, true, false))
.WillOnce(Return(SQL_SUCCESS));

FAILOVER_HANDLER failover_handler(dbc, ds, mock_connection_handler, mock_ts, mock_metrics);
Expand Down Expand Up @@ -490,7 +490,7 @@ TEST_F(FailoverHandlerTest, GetRdsClusterHostUrl) {
TEST_F(FailoverHandlerTest, ConnectToNewWriter) {
SQLCHAR server[] = "my-cluster-name.cluster-XYZ.us-east-2.rds.amazonaws.com";

EXPECT_CALL(*mock_connection_handler, do_connect(dbc, ds, _))
EXPECT_CALL(*mock_connection_handler, do_connect_impl(dbc, ds, _, false))
.WillOnce(Return(SQL_SUCCESS))
.WillOnce(Return(SQL_SUCCESS));

Expand Down
30 changes: 15 additions & 15 deletions unit_testing/failover_reader_handler_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ TEST_F(FailoverReaderHandlerTest, BuildHostsList) {
TEST_F(FailoverReaderHandlerTest, GetConnectionFromHosts_Failure) {
EXPECT_CALL(*mock_ts, get_topology(_, true)).WillRepeatedly(Return(topology));

EXPECT_CALL(*mock_connection_handler, connect(_, nullptr)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect_impl(_, nullptr, false)).WillRepeatedly(Return(nullptr));

EXPECT_CALL(*mock_ts, mark_host_down(reader_a_host)).Times(1);
EXPECT_CALL(*mock_ts, mark_host_down(reader_b_host)).Times(1);
Expand All @@ -247,8 +247,8 @@ TEST_F(FailoverReaderHandlerTest, GetConnectionFromHosts_Success_Reader) {

EXPECT_CALL(*mock_reader_a_proxy, is_connected()).WillRepeatedly(Return(true));

EXPECT_CALL(*mock_connection_handler, connect(_, nullptr)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect(reader_a_host, nullptr)).WillRepeatedly(Return(mock_reader_a_proxy));
EXPECT_CALL(*mock_connection_handler, connect_impl(_, nullptr, false)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect_impl(reader_a_host, nullptr, false)).WillRepeatedly(Return(mock_reader_a_proxy));

// Reader C will not be used as it is put at the end. Will only try to connect to A and B
EXPECT_CALL(*mock_ts, mark_host_up(reader_a_host)).Times(1);
Expand All @@ -274,8 +274,8 @@ TEST_F(FailoverReaderHandlerTest, GetConnectionFromHosts_Success_Writer) {

EXPECT_CALL(*mock_writer_proxy, is_connected()).WillRepeatedly(Return(true));

EXPECT_CALL(*mock_connection_handler, connect(_, nullptr)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect(writer_host, nullptr)).WillRepeatedly(Return(mock_writer_proxy));
EXPECT_CALL(*mock_connection_handler, connect_impl(_, nullptr, false)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect_impl(writer_host, nullptr, false)).WillRepeatedly(Return(mock_writer_proxy));

EXPECT_CALL(*mock_ts, mark_host_up(writer_host)).Times(1);

Expand Down Expand Up @@ -309,9 +309,9 @@ TEST_F(FailoverReaderHandlerTest, GetConnectionFromHosts_FastestHost) {
EXPECT_CALL(*mock_reader_a_proxy, is_connected()).WillRepeatedly(Return(true));
EXPECT_CALL(*mock_reader_b_proxy, is_connected()).WillRepeatedly(Return(true));

EXPECT_CALL(*mock_connection_handler, connect(_, nullptr)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect(reader_a_host, nullptr)).WillRepeatedly(Return(mock_reader_a_proxy));
EXPECT_CALL(*mock_connection_handler, connect(reader_b_host, nullptr)).WillRepeatedly(Invoke([&]() {
EXPECT_CALL(*mock_connection_handler, connect_impl(_, nullptr, false)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect_impl(reader_a_host, nullptr, false)).WillRepeatedly(Return(mock_reader_a_proxy));
EXPECT_CALL(*mock_connection_handler, connect_impl(reader_b_host, nullptr, false)).WillRepeatedly(Invoke([&]() {
std::this_thread::sleep_for(std::chrono::milliseconds(5000));
return mock_reader_b_proxy;
}));
Expand Down Expand Up @@ -342,12 +342,12 @@ TEST_F(FailoverReaderHandlerTest, GetConnectionFromHosts_Timeout) {
EXPECT_CALL(*mock_reader_b_proxy, is_connected()).WillRepeatedly(Return(true));
EXPECT_CALL(*mock_reader_b_proxy, mock_connection_proxy_destructor());

EXPECT_CALL(*mock_connection_handler, connect(_, nullptr)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect(reader_a_host, nullptr)).WillRepeatedly(Invoke([&]() {
EXPECT_CALL(*mock_connection_handler, connect_impl(_, nullptr, false)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect_impl(reader_a_host, nullptr, false)).WillRepeatedly(Invoke([&]() {
std::this_thread::sleep_for(std::chrono::milliseconds(5000));
return mock_reader_a_proxy;
}));
EXPECT_CALL(*mock_connection_handler, connect(reader_b_host, nullptr)).WillRepeatedly(Invoke([&]() {
EXPECT_CALL(*mock_connection_handler, connect_impl(reader_b_host, nullptr, false)).WillRepeatedly(Invoke([&]() {
std::this_thread::sleep_for(std::chrono::milliseconds(5000));
return mock_reader_b_proxy;
}));
Expand All @@ -368,7 +368,7 @@ TEST_F(FailoverReaderHandlerTest, GetConnectionFromHosts_Timeout) {
TEST_F(FailoverReaderHandlerTest, Failover_Failure) {
EXPECT_CALL(*mock_ts, get_topology(_, true)).WillRepeatedly(Return(topology));

EXPECT_CALL(*mock_connection_handler, connect(_, nullptr)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect_impl(_, nullptr, false)).WillRepeatedly(Return(nullptr));

EXPECT_CALL(*mock_ts, mark_host_down(reader_a_host)).Times(AtLeast(1));
EXPECT_CALL(*mock_ts, mark_host_down(reader_b_host)).Times(AtLeast(1));
Expand Down Expand Up @@ -404,10 +404,10 @@ TEST_F(FailoverReaderHandlerTest, Failover_Success_Reader) {
EXPECT_CALL(*mock_reader_b_proxy, is_connected()).WillRepeatedly(Return(true));
EXPECT_CALL(*mock_reader_b_proxy, mock_connection_proxy_destructor());

EXPECT_CALL(*mock_connection_handler, connect(_, nullptr)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect(reader_a_host, nullptr)).WillRepeatedly(
EXPECT_CALL(*mock_connection_handler, connect_impl(_, nullptr, false)).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*mock_connection_handler, connect_impl(reader_a_host, nullptr, false)).WillRepeatedly(
Return(mock_reader_a_proxy));
EXPECT_CALL(*mock_connection_handler, connect(reader_b_host, nullptr)).WillRepeatedly(Invoke([=]() {
EXPECT_CALL(*mock_connection_handler, connect_impl(reader_b_host, nullptr, false)).WillRepeatedly(Invoke([=]() {
std::this_thread::sleep_for(std::chrono::milliseconds(5000));
return mock_reader_b_proxy;
}));
Expand Down
Loading