From eb02b5418286457f427622b9643249cb26fde052 Mon Sep 17 00:00:00 2001 From: ganeshmurthy Date: Wed, 29 Jan 2025 16:32:29 -0500 Subject: [PATCH] Fixes #1723: Force dataConnectionCount to 1 if set to zero. Changed type of data_connection_count to int --- include/qpid/dispatch/router_core.h | 8 ---- .../skupper_router/management/skrouter.json | 2 +- src/adaptors/amqp/connection_manager.c | 37 +------------------ src/adaptors/amqp/server_config.c | 8 +--- src/adaptors/amqp/server_config.h | 4 -- src/dispatch.c | 25 +++++++++++-- src/dispatch_private.h | 25 ++++++------- src/router_core/router_core.c | 4 -- tests/system_tests_one_router.py | 5 ++- 9 files changed, 41 insertions(+), 77 deletions(-) diff --git a/include/qpid/dispatch/router_core.h b/include/qpid/dispatch/router_core.h index e8eeb5f23..57e6fcae9 100644 --- a/include/qpid/dispatch/router_core.h +++ b/include/qpid/dispatch/router_core.h @@ -74,14 +74,6 @@ qd_dispatch_t *qdr_core_dispatch(qdr_core_t *core); */ void qdr_process_tick(qdr_core_t *core); -/** - * Return the count of worker threads. - * - * @param core Pointer to the core object returned by qd_core() - * @return count of worker threads - */ -int qdr_core_get_worker_thread_count(const qdr_core_t *core); - /** * @brief Return the text of the router's virtual application network ID, or 0. * diff --git a/python/skupper_router/management/skrouter.json b/python/skupper_router/management/skrouter.json index 350de365d..29a8bd088 100644 --- a/python/skupper_router/management/skrouter.json +++ b/python/skupper_router/management/skrouter.json @@ -618,7 +618,7 @@ "required": false }, "dataConnectionCount": { - "description": "The number of parallel data connections to carry streaming data between routers. Applies only to interior routers", + "description": "The number of parallel data connections to carry streaming data between routers. Applies only to interior routers and to connectors with a role of 'inter-router'", "type": "string", "required": false, "default": "auto", diff --git a/src/adaptors/amqp/connection_manager.c b/src/adaptors/amqp/connection_manager.c index 7b7ca9e81..7b83209fe 100644 --- a/src/adaptors/amqp/connection_manager.c +++ b/src/adaptors/amqp/connection_manager.c @@ -267,15 +267,6 @@ QD_EXPORT qd_error_t qd_entity_refresh_connector(qd_entity_t* entity, void *impl } -// This is used to calculate inter-router data connection -// count when the user explicitly requests 'auto' -// or lets it default to that. -static int auto_calc_connection_count(qd_dispatch_t *qd) -{ - return (qdr_core_get_worker_thread_count(qd_dispatch_router_core(qd)) + 1) / 2; -} - - QD_EXPORT qd_connector_t *qd_dispatch_configure_connector(qd_dispatch_t *qd, qd_entity_t *entity) { qd_connection_manager_t *cm = qd->connection_manager; @@ -309,38 +300,12 @@ QD_EXPORT qd_connector_t *qd_dispatch_configure_connector(qd_dispatch_t *qd, qd_ } } - uint32_t connection_count = 0; - // - // If the user asks for automatic setting of the number - // of data connnection count, make one data connection - // for every two worker threads. This is the best ratio - // for high load, as determined by throughput performance - // testing. - // - if (!strcmp("auto", ct->config.data_connection_count)) { - // The user has explicitly requested 'auto'. - connection_count = auto_calc_connection_count(qd); - qd_log(LOG_CONN_MGR, QD_LOG_INFO, "Inter-router data connections calculated at %d ", - connection_count); - } else if (1 == sscanf(ct->config.data_connection_count, "%u", &connection_count)) { - // The user has requested a specific number of connections. - qd_log(LOG_CONN_MGR, QD_LOG_INFO, "Inter-router data connections set to %d ", connection_count); - } else { - // The user has entered a non-numeric value that is not 'auto'. - // This is not a legal value. Default to 'auto' and mention it. - qd_log(LOG_CONN_MGR, QD_LOG_INFO, "Bad value \"%s\" for dataConnectionCount ", - ct->config.data_connection_count); - connection_count = auto_calc_connection_count(qd); - qd_log(LOG_CONN_MGR, QD_LOG_INFO, "Inter-router data connections calculated at %d ", - connection_count); - } - // // If this connection has a data-connection-group, set up the group members now // if (ct->config.has_data_connectors) { qd_generate_discriminator(ct->group_correlator); - for (uint32_t i = 0; i < connection_count; i++) { + for (int i = 0; i < qd->data_connection_count; i++) { qd_connector_t *dc = qd_server_connector(qd->server); if (!dc) { qd_error(QD_ERROR_CONFIG, "Failed to create data Connector %s: resource allocation failed", ct->config.name); diff --git a/src/adaptors/amqp/server_config.c b/src/adaptors/amqp/server_config.c index 0856b6d07..86e7fba78 100644 --- a/src/adaptors/amqp/server_config.c +++ b/src/adaptors/amqp/server_config.c @@ -152,14 +152,9 @@ qd_error_t qd_server_config_load(qd_dispatch_t *qd, qd_server_config_t *config, if (strcmp(config->role, "inter-router") == 0) { // For inter-router connections only, the dataConnectionCount defaults to "auto", // which means it will be determined as a function of the number of worker threads. - config->data_connection_count = strdup(qd->data_connection_count); // If the user has *not* explicitly set the value "0", // then we will have some data connections. - if (strcmp(config->data_connection_count, "0")) { - config->has_data_connectors = true; - } - } else { - config->data_connection_count = strdup("0"); + config->has_data_connectors = true; } set_config_host(config, entity); @@ -281,7 +276,6 @@ void qd_server_config_free(qd_server_config_t *cf) if (cf->log_message) free(cf->log_message); if (cf->policy_vhost) free(cf->policy_vhost); - free(cf->data_connection_count); if (cf->conn_props) pn_data_free(cf->conn_props); memset(cf, 0, sizeof(*cf)); diff --git a/src/adaptors/amqp/server_config.h b/src/adaptors/amqp/server_config.h index 73ac109ef..cb2025af7 100644 --- a/src/adaptors/amqp/server_config.h +++ b/src/adaptors/amqp/server_config.h @@ -268,10 +268,6 @@ typedef struct qd_server_config_t { */ pn_data_t *conn_props; - /** - * For inter-router roles only. The number of data connections associated with the link. - */ - char *data_connection_count; bool has_data_connectors; /** diff --git a/src/dispatch.c b/src/dispatch.c index dd0a427e4..f2adccf9c 100644 --- a/src/dispatch.c +++ b/src/dispatch.c @@ -238,9 +238,29 @@ qd_error_t qd_dispatch_configure_router(qd_dispatch_t *qd, qd_entity_t *entity) strcpy(qd->router_id, mode); qd_generate_discriminator(qd->router_id + strlen(qd->router_id)); } - qd->thread_count = qd_entity_opt_long(entity, "workerThreads", 4); QD_ERROR_RET(); - qd->data_connection_count = qd_entity_opt_string(entity, "dataConnectionCount", "auto"); QD_ERROR_RET(); + char *data_conn_count_str = qd_entity_opt_string(entity, "dataConnectionCount", "auto"); QD_ERROR_RET(); + + if (!strcmp("auto", data_conn_count_str)) { + // The user has explicitly requested 'auto'. + qd->data_connection_count = (qd->thread_count + 1) / 2; + qd_log(LOG_ROUTER, QD_LOG_INFO, "Inter-router data connections calculated at %d ", qd->data_connection_count); + } else if (1 == sscanf(data_conn_count_str, "%u", &qd->data_connection_count)) { + // The user has requested a specific number of connections. + if (qd->data_connection_count == 0) { + // Force data_connection_count to 1 if set to 0. + qd->data_connection_count = 1; + } + qd_log(LOG_ROUTER, QD_LOG_INFO, "Inter-router data connections set to %d ", qd->data_connection_count); + } else { + // The user has entered a non-numeric value that is not 'auto'. + // This is not a legal value. Default to 'auto' and mention it. + qd_log(LOG_ROUTER, QD_LOG_INFO, "Bad value \"%s\" for dataConnectionCount ", data_conn_count_str); + qd->data_connection_count = (qd->thread_count + 1) / 2; + qd_log(LOG_ROUTER, QD_LOG_INFO, "Inter-router data connections calculated at %d ", qd->data_connection_count); + } + free(data_conn_count_str); + qd->timestamps_in_utc = qd_entity_opt_bool(entity, "timestampsInUTC", false); QD_ERROR_RET(); qd->timestamp_format = qd_entity_opt_string(entity, "timestampFormat", 0); QD_ERROR_RET(); qd->metadata = qd_entity_opt_string(entity, "metadata", 0); QD_ERROR_RET(); @@ -385,7 +405,6 @@ void qd_dispatch_free(qd_dispatch_t *qd) qd_http_server_free(qd_server_http(qd->server)); free(qd->sasl_config_path); - free(qd->data_connection_count); free(qd->sasl_config_name); qd_connection_manager_free(qd->connection_manager); qd_policy_free(qd->policy); diff --git a/src/dispatch_private.h b/src/dispatch_private.h index e3e2a7708..9afc2fece 100644 --- a/src/dispatch_private.h +++ b/src/dispatch_private.h @@ -46,19 +46,18 @@ struct qd_dispatch_t { qd_connection_manager_t *connection_manager; qd_policy_t *policy; qd_address_treatment_t default_treatment; - - int thread_count; - char *sasl_config_path; - char *sasl_config_name; - char *router_area; - char *router_id; - qd_router_mode_t router_mode; - char *van_id; - char *timestamp_format; - char *metadata; - bool timestamps_in_utc; - char *data_connection_count; - bool terminate_tcp_conns; + qd_router_mode_t router_mode; + int thread_count; + uint32_t data_connection_count; + char *sasl_config_path; + char *sasl_config_name; + char *router_area; + char *router_id; + char *van_id; + char *timestamp_format; + char *metadata; + bool timestamps_in_utc; + bool terminate_tcp_conns; }; qd_dispatch_t *qd_dispatch_get_dispatch(void); diff --git a/src/router_core/router_core.c b/src/router_core/router_core.c index 4c1c9909c..e372631e9 100644 --- a/src/router_core/router_core.c +++ b/src/router_core/router_core.c @@ -390,10 +390,6 @@ void qdr_router_node_free(qdr_core_t *core, qdr_node_t *rnode) free_qdr_node_t(rnode); } -int qdr_core_get_worker_thread_count(const qdr_core_t *core) -{ - return core->worker_thread_count; -} const char *qdr_core_van_id(const qdr_core_t *core) { diff --git a/tests/system_tests_one_router.py b/tests/system_tests_one_router.py index ca09bd68a..6301a5ca7 100644 --- a/tests/system_tests_one_router.py +++ b/tests/system_tests_one_router.py @@ -3588,7 +3588,10 @@ def test_60_threads_vs_data_connection_count(self): expected_result = int((int(worker_threads) + 1) / 2) msg = "Inter-router data connections calculated at " + str(expected_result) else: - expected_result = con_count + if con_count == '0': + expected_result = '1' + else: + expected_result = con_count msg = "Inter-router data connections set to " + str(expected_result) print("worker threads:", worker_threads, ", requested connections:", con_count, ", expected result:", expected_result, end=" ")