Skip to content

Port remove hndl->cluster, check for default, don't ask sockpool for … #5554

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

Merged
merged 2 commits into from
Nov 21, 2025
+85 −44
Merged

Port remove hndl->cluster, check for default, don't ask sockpool for … #5554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
827dce9
Port remove hndl->cluster, check for default, don't ask sockpool for …
chands10 Nov 6, 2025
e0fecbb
Update api_events test
chands10 Nov 7, 2025
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
124 changes: 83 additions & 41 deletions cdb2api/cdb2api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1624,8 +1624,8 @@ static void read_comdb2db_environment_cfg(cdb2_hndl_tp *hndl, const char *comdb2
}

if (hndl) {
if ((strcasecmp(hndl->cluster, "default") == 0) && cdb2_default_cluster_set_from_env)
strcpy(hndl->cluster, cdb2_default_cluster);
if ((strcasecmp(hndl->type, "default") == 0) && cdb2_default_cluster_set_from_env)
strcpy(hndl->type, cdb2_default_cluster);
if (cdb2_connect_timeout_set_from_env)
hndl->connect_timeout = CDB2_CONNECT_TIMEOUT;
if (cdb2_sockpool_send_timeoutms_set_from_env)
Expand All @@ -1649,7 +1649,7 @@ static void read_comdb2db_environment_cfg(cdb2_hndl_tp *hndl, const char *comdb2
pthread_mutex_unlock(&cdb2_sockpool_mutex);
}

static void only_read_config(cdb2_hndl_tp *); /* FORWARD */
static void only_read_config(cdb2_hndl_tp *, int *); /* FORWARD */

static int cdb2_max_room_num = 0;
static int cdb2_has_room_distance = 0;
Expand Down Expand Up @@ -1814,8 +1814,8 @@ static void read_comdb2db_cfg(cdb2_hndl_tp *hndl, SBUF2 *s, const char *comdb2db
if (!cdb2_default_cluster_set_from_env && strcasecmp("default_type", tok) == 0) {
tok = strtok_r(NULL, " :,", &last);
if (tok) {
if (hndl && (strcasecmp(hndl->cluster, "default") == 0)) {
strncpy(hndl->cluster, tok, sizeof(cdb2_default_cluster) - 1);
if (hndl && (strcasecmp(hndl->type, "default") == 0)) {
strncpy(hndl->type, tok, sizeof(cdb2_default_cluster) - 1);
} else if (!hndl) {
strncpy(cdb2_default_cluster, tok, sizeof(cdb2_default_cluster) - 1);
}
Expand Down Expand Up @@ -2240,6 +2240,11 @@ static int read_available_comdb2db_configs(cdb2_hndl_tp *hndl, char comdb2db_hos
#ifdef CDB2API_TEST
}
#endif

if (hndl && strcasecmp(hndl->type, "default") == 0 && cdb2_default_cluster[0] != '\0') {
strncpy(hndl->type, cdb2_default_cluster, sizeof(hndl->type) - 1);
}

pthread_mutex_unlock(&cdb2_cfg_lock);
return 0;
}
Expand Down Expand Up @@ -3072,6 +3077,20 @@ int cdb2_socket_pool_get_fd(cdb2_hndl_tp *hndl, const char *typestr, int dbnum,
return fd;
}

static int typestr_type_is_default(const char *typestr)
{
const int TYPESTR_TIER_IDX = 2;
const char *type_start = typestr;
for (int i = 0; i < TYPESTR_TIER_IDX; i++) {
type_start = strchr(type_start, '/');
assert(type_start);
type_start++;
}
const char *const type_end = strchr(type_start, '/');
assert(type_end);
return strncmp(type_start, "default", type_end - type_start) == 0;
}

/* Get the sbuf of a socket matching the given type string from
* the pool. Returns NULL if none is available or the sbuf on
* success. */
Expand All @@ -3089,6 +3108,13 @@ SBUF2 *cdb2_socket_pool_get(cdb2_hndl_tp *hndl, const char *typestr, int dbnum,
}
#endif

#ifdef CDB2API_TEST
if (typestr_type_is_default(typestr)) {
fprintf(stderr, "%s: ERR: Did not expect default type in typestring\n", __func__);
abort();
}
#endif

if (was_from_local_cache) {
*was_from_local_cache = 0;
sb = local_connection_cache_get(hndl, typestr);
Expand All @@ -3112,6 +3138,13 @@ void cdb2_socket_pool_donate_ext(const cdb2_hndl_tp *hndl, const char *typestr,
{
if (log_calls)
fprintf(stderr, "%p> %s(%s,%d): fd=%d\n", (void *)pthread_self(), __func__, typestr, dbnum, fd);
#ifdef CDB2API_TEST
if (typestr_type_is_default(typestr)) {
fprintf(stderr, "%s: ERR: Did not expect default type in typestring\n", __func__);
abort();
}
#endif

int enabled = 0;
int sockpool_fd = -1;
int sp_generation = -1;
Expand Down Expand Up @@ -7769,9 +7802,17 @@ static int cdb2_dbinfo_query(cdb2_hndl_tp *hndl, const char *type, const char *d
return rc;
}

static inline void only_read_config(cdb2_hndl_tp *hndl)
static inline void only_read_config(cdb2_hndl_tp *hndl, int *default_err)
{
read_available_comdb2db_configs(NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL);
if (default_err && strcasecmp(hndl->type, "default") == 0) {
Copy link
Contributor Author

@chands10 chands10 Nov 6, 2025

Choose a reason for hiding this comment

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

Added that we only do this check if default_err is defined

Copy link
Contributor

@morgando morgando Nov 20, 2025

Choose a reason for hiding this comment

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

👍

if (cdb2_default_cluster[0] != '\0') {
strncpy(hndl->type, cdb2_default_cluster, sizeof(hndl->type) - 1);
} else {
snprintf(hndl->errstr, sizeof(hndl->errstr), "No default_type entry in comdb2db config.\n");
*default_err = -1;
}
}
set_cdb2_timeouts(hndl);
}

Expand All @@ -7797,6 +7838,11 @@ static int cdb2_get_dbhosts(cdb2_hndl_tp *hndl)
&comdb2db_num, hndl->dbname, hndl->hosts, &(hndl->num_hosts), &hndl->dbnum, 0, hndl->shards,
&(hndl->num_shards));

if (strcasecmp(hndl->type, "default") == 0) {
sprintf(hndl->errstr, "No default_type entry in comdb2db config.\n");
return -1;
}

/* Before database destination discovery */
cdb2_event *e = NULL;
void *callbackrc;
Expand Down Expand Up @@ -7836,20 +7882,7 @@ static int cdb2_get_dbhosts(cdb2_hndl_tp *hndl)
strcpy(comdb2db_name, cdb2_comdb2dbname);
}

if (strcasecmp(hndl->cluster, "default") == 0) {
if (cdb2_default_cluster[0] == '\0') {
sprintf(hndl->errstr, "cdb2_get_dbhosts: no default_type "
"entry in comdb2db config.");
rc = -1;
goto after_callback;
}
strncpy(hndl->cluster, cdb2_default_cluster, sizeof(hndl->cluster) - 1);
if (cdb2cfg_override || (cdb2_use_env_vars && default_type_override_env)) {
strncpy(hndl->type, cdb2_default_cluster, sizeof(hndl->type) - 1);
}
}

if (strcasecmp(hndl->cluster, "local") == 0) {
if (strcasecmp(hndl->type, "local") == 0) {
hndl->num_hosts = 1;
strcpy(hndl->hosts[0], "localhost");
hndl->flags |= CDB2_DIRECT_CPU;
Expand Down Expand Up @@ -7915,7 +7948,7 @@ static int cdb2_get_dbhosts(cdb2_hndl_tp *hndl)
if (i == master)
continue;
rc = comdb2db_get_dbhosts(hndl, comdb2db_name, comdb2db_num, comdb2db_hosts[i], comdb2db_ports[i],
hndl->hosts, &hndl->num_hosts, hndl->dbname, hndl->cluster, &hndl->dbnum,
hndl->hosts, &hndl->num_hosts, hndl->dbname, hndl->type, &hndl->dbnum,
&hndl->num_hosts_sameroom, num_retry, use_bmsd, hndl->shards, &hndl->num_shards,
&hndl->num_shards_sameroom);
if (rc == 0 || time(NULL) >= max_time) {
Expand All @@ -7928,7 +7961,7 @@ static int cdb2_get_dbhosts(cdb2_hndl_tp *hndl)
}
if (rc == -1 && time(NULL) < max_time) {
rc = comdb2db_get_dbhosts(hndl, comdb2db_name, comdb2db_num, comdb2db_hosts[master], comdb2db_ports[master],
hndl->hosts, &hndl->num_hosts, hndl->dbname, hndl->cluster, &hndl->dbnum,
hndl->hosts, &hndl->num_hosts, hndl->dbname, hndl->type, &hndl->dbnum,
&hndl->num_hosts_sameroom, num_retry, use_bmsd, hndl->shards, &hndl->num_shards,
&hndl->num_shards_sameroom);
}
Expand All @@ -7943,9 +7976,10 @@ static int cdb2_get_dbhosts(cdb2_hndl_tp *hndl)
goto after_callback;
}
if (hndl->num_hosts == 0) {
sprintf(hndl->errstr, "cdb2_get_dbhosts: comdb2db has no entry of "
"db %s of cluster type %s.",
hndl->dbname, hndl->cluster);
sprintf(hndl->errstr,
"cdb2_get_dbhosts: comdb2db has no entry of "
"db %s of cluster type %s.",
hndl->dbname, hndl->type);
rc = -1;
goto after_callback;
}
Expand Down Expand Up @@ -8084,10 +8118,10 @@ static void after_discovery(cdb2_hndl_tp *hndl)
}
}

static int get_connection_int(cdb2_hndl_tp *hndl)
static int get_connection_int(cdb2_hndl_tp *hndl, int *err)
{
only_read_config(hndl);
if (get_dbinfo)
only_read_config(hndl, err);
if (get_dbinfo || *err)
return -1;
before_discovery(hndl);
SBUF2 *sb = sockpool_get(hndl);
Expand All @@ -8097,7 +8131,7 @@ static int get_connection_int(cdb2_hndl_tp *hndl)
return init_connection(hndl, sb);
}

static int get_connection(cdb2_hndl_tp *hndl)
static int get_connection(cdb2_hndl_tp *hndl, int *err)
{
if (hndl->is_admin || (hndl->flags & CDB2_MASTER)) // don't grab from sockpool
return -1;
Expand All @@ -8117,7 +8151,7 @@ static int get_connection(cdb2_hndl_tp *hndl)
} else if (get_dbinfo || sockpool_enabled == -1 || cdb2cfg_override) {
return -1;
}
int rc = get_connection_int(hndl);
int rc = get_connection_int(hndl, err);
return rc;
}

Expand Down Expand Up @@ -8174,7 +8208,7 @@ static int configure_from_literal(cdb2_hndl_tp *hndl, const char *type)
assert(type_copy[0] == '@');
char *s = type_copy + 1; // advance past the '@'

only_read_config(hndl);
only_read_config(hndl, NULL); // don't care about default here

char *machine;
machine = strtok_r(s, ",", &eomachine);
Expand Down Expand Up @@ -8522,8 +8556,7 @@ static cdb2_ssl_sess *cdb2_get_ssl_sessions(cdb2_hndl_tp *hndl)
return NULL;

for (pos = cdb2_ssl_sess_cache.next; pos != NULL; pos = pos->next) {
if (strcasecmp(hndl->dbname, pos->dbname) == 0 &&
strcasecmp(hndl->cluster, pos->cluster) == 0) {
if (strcasecmp(hndl->dbname, pos->dbname) == 0 && strcasecmp(hndl->type, pos->cluster) == 0) {
if (!pos->ref) {
pos->ref = 1;
break;
Expand Down Expand Up @@ -8562,8 +8595,8 @@ static int cdb2_add_ssl_session(cdb2_hndl_tp *hndl)
return ENOMEM;
strncpy(hndl->sess->dbname, hndl->dbname, sizeof(hndl->dbname) - 1);
hndl->sess->dbname[sizeof(hndl->dbname) - 1] = '\0';
strncpy(hndl->sess->cluster, hndl->cluster, sizeof(hndl->cluster) - 1);
hndl->sess->cluster[sizeof(hndl->cluster) - 1] = '\0';
strncpy(hndl->sess->cluster, hndl->type, sizeof(hndl->type) - 1);
hndl->sess->cluster[sizeof(hndl->type) - 1] = '\0';
hndl->sess->ref = 1;
hndl->sess->sessobj = NULL;

Expand Down Expand Up @@ -8618,7 +8651,6 @@ int cdb2_open(cdb2_hndl_tp **handle, const char *dbname, const char *type,
hndl->gbl_event_version = calloc(1, sizeof(*hndl->gbl_event_version));
TAILQ_INIT(&hndl->queries);
strncpy(hndl->dbname, dbname, sizeof(hndl->dbname) - 1);
strncpy(hndl->cluster, type, sizeof(hndl->cluster) - 1);
strncpy(hndl->type, type, sizeof(hndl->type) - 1);
hndl->flags = flags;
hndl->dbnum = 1;
Expand Down Expand Up @@ -8659,8 +8691,7 @@ int cdb2_open(cdb2_hndl_tp **handle, const char *dbname, const char *type,
fprintf(stderr, "WARNING: %s:Value of %s is not valid. Using value '%s'.\n", __func__,
COMDB2_CONFIG_DB_DEFAULT_TYPE, db_default_type);
}
strncpy(hndl->cluster, db_default_type, sizeof(hndl->cluster) - 1);
strncpy(hndl->type, hndl->cluster, sizeof(hndl->type) - 1);
strncpy(hndl->type, db_default_type, sizeof(hndl->type) - 1);
hndl->db_default_type_override_env = 1;
}
}
Expand Down Expand Up @@ -8699,7 +8730,9 @@ int cdb2_open(cdb2_hndl_tp **handle, const char *dbname, const char *type,

if (hndl->flags & CDB2_DIRECT_CPU) {
/* Get defaults from comdb2db.cfg */
only_read_config(hndl);
only_read_config(hndl, &rc);
if (rc)
goto out;
hndl->got_dbinfo = 1;
hndl->num_hosts = 1;
strncpy(hndl->hosts[0], type, sizeof(hndl->hosts[0]) - 1);
Expand Down Expand Up @@ -8727,7 +8760,10 @@ int cdb2_open(cdb2_hndl_tp **handle, const char *dbname, const char *type,
hndl->sockpool_enabled = db_host_info ? -1 : 0;
}

if (get_connection(hndl) != 0) {
const int should_get_dbinfo = get_connection(hndl, &rc);
if (rc)
goto out;
if (should_get_dbinfo) {
hndl->got_dbinfo = 1;
rc = cdb2_get_dbhosts(hndl);
}
Expand Down Expand Up @@ -9047,7 +9083,7 @@ static void *cdb2_invoke_callback(cdb2_hndl_tp *hndl, cdb2_event *e, int argc,
void *rc;
int state;
char *fp;
const char *dbtype = (hndl == NULL) ? NULL : hndl->type;
const char *dbtype;

/* Fast return if no arguments need to be passed to the callback. */
if (e->argc == 0)
Expand All @@ -9069,6 +9105,12 @@ static void *cdb2_invoke_callback(cdb2_hndl_tp *hndl, cdb2_event *e, int argc,
hostname = hndl->hosts[hndl->connected_host];
port = hndl->ports[hndl->connected_host];
}

if (hndl == NULL || strcasecmp(hndl->type, "default") == 0)
dbtype = cdb2_default_cluster;
else
dbtype = hndl->type;

rc = 0;
state = 0;

Expand Down
1 change: 0 additions & 1 deletion cdb2api/cdb2api_hndl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ typedef struct cdb2_ssl_sess cdb2_ssl_sess;

struct cdb2_hndl {
char dbname[DBNAME_LEN];
char cluster[64];
char type[TYPE_LEN];
char hosts[MAX_NODES][CDB2HOSTNAME_LEN];
uint64_t timestampus; // client query timestamp of first try
Expand Down
2 changes: 1 addition & 1 deletion tests/api_events.test/expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ FP is 4f16a8ec9db90f803e406659938b2602
RC is 0
RC is 1
====== VERIFYING DBTYPE ARG ======
DBTYPE is unresolved
DBTYPE is resolved
DBTYPE is resolved
DBTYPE is resolved
DBTYPE is resolved
Expand Down
2 changes: 1 addition & 1 deletion tests/tools/api_events.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ static int TEST_dbtype_arg(const char *db, const char *tier)
e1 = cdb2_register_event(NULL, CDB2_BEFORE_DISCOVERY, 0, my_dbtype_hook, NULL, 1, CDB2_DBTYPE);
e2 = cdb2_register_event(NULL, CDB2_AT_OPEN, 0, my_dbtype_hook, NULL, 1, CDB2_DBTYPE);

/* default */
/* actual */
cdb2_open(&h, db, tier, 0);
cdb2_close(h);
/* actual */
Expand Down