-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Description
Parent Issue
Part of #5479 — Unit Testing Milestone 3: Extract Testable Logic from Coupled Classes
Context
When ProxySQL routes a query to a hostgroup, it must select one server from potentially many. The selection considers weights, server status (ONLINE/SHUNNED/OFFLINE), max_connections, max_replication_lag, and max_latency. This algorithm is currently embedded inside `get_connection()` / `get_random_MySrvC()` in the HostGroups Manager, coupled to real connection creation.
Extracting the selection algorithm would enable testing weighted distribution, failover behavior, and health-based exclusion without real network connections.
Current Code
File: `lib/Base_HostGroups_Manager.cpp` (templated)
The server selection function:
- Iterates through servers in a hostgroup
- Filters out servers that are OFFLINE_HARD, SHUNNED, or at max_connections
- Filters by replication lag and latency thresholds
- Selects from remaining candidates using weighted random selection
File: `include/Base_HostGroups_Manager.h`
- `MySrvC` struct contains: address, port, weight, max_connections, status, max_replication_lag, max_latency_us, ConnectionsUsed, ConnectionsFree
Scope
Extract:
`select_server_from_candidates()` — Pure selection algorithm:
```cpp
struct ServerCandidate {
int index; // index into the server list
int64_t weight;
enum MySerStatus status;
unsigned int current_connections;
unsigned int max_connections;
unsigned int current_replication_lag;
unsigned int max_replication_lag;
unsigned int current_latency_us;
unsigned int max_latency_us;
};
/**
- @brief Select a server from a list of candidates using weighted random selection.
- @param candidates Array of server candidates
- @param count Number of candidates
- @param random_seed Seed for deterministic testing
- @return Index of selected server, or -1 if no eligible server found
/
int select_server_from_candidates(
const ServerCandidate candidates,
int count,
unsigned int random_seed
);
```
Unit tests should cover:
- Single server → always selected
- Two servers equal weight → roughly 50/50 distribution (statistical test)
- Two servers 3:1 weight → roughly 75/25 distribution
- Server at max_connections → skipped
- OFFLINE_HARD server → never selected
- OFFLINE_SOFT server → not selected for new connections
- SHUNNED server → not selected
- All servers offline → returns -1
- Server exceeding max_replication_lag → excluded
- Server exceeding max_latency → excluded
- Weight=0 server → never selected
- Large number of servers (100+) → distribution matches weights
IMPORTANT: Implementation Instructions
Branch and PR rules
- Create your branch from `v3.0-5473`, NOT from `v3.0`
- Target the PR to `v3.0-5473`, NOT to `v3.0`
- Branch name should be `v3.0-5492`
Where to place files
- Extracted function declarations: create `include/ServerSelection.h` with include guard (follow the pattern of `include/ConnectionPoolDecision.h`)
- Extracted function implementations: place in an appropriate `.cpp` file in `lib/`
- Test file MUST go in `test/tap/tests/unit/server_selection_unit-t.cpp`
How to write the test file
The project has a unit test harness. Your test file MUST:
-
Include the harness headers:
```cpp
#include "tap.h"
#include "test_globals.h"
#include "test_init.h"
``` -
Include proxysql headers for the types you need:
```cpp
#include "proxysql.h"
#include "ServerSelection.h" // your new header
``` -
Call `test_init_minimal()` at the start of `main()`
-
Call `test_cleanup_minimal()` at the end
-
Use TAP functions: `plan()`, `ok()`, `exit_status()`
DO NOT:
- Do NOT reimplement the extracted functions in the test file — the test must call the REAL functions from `libproxysql.a`
- Do NOT manually define `noise_failures`, `noise_failure_mutex`, `stop_noise_tools`, or `get_noise_tools_count` — these are already provided by `test_globals.cpp`
- Do NOT place the test in `test/tap/tests/` — it MUST go in `test/tap/tests/unit/`
- Do NOT merge `v3.0-5473` into your branch — rebase instead if needed
How to register the test in the Makefile
Edit `test/tap/tests/unit/Makefile`:
- Add the test name to the `UNIT_TESTS` variable (append to existing list)
- Add a build rule following the existing pattern:
```makefile
server_selection_unit-t: server_selection_unit-t.cpp$(TEST_HELPERS_OBJ) $ (LIBPROXYSQLAR)
$(CXX) $ <$(TEST_HELPERS_OBJ) $ (IDIRS)$(LDIRS) $ (OPT) \
$(LIBPROXYSQLAR_FULL) $ (STATIC_LIBS) $(MYLIBS) \
$(ALLOW_MULTI_DEF) -o $ @
```
Reference examples
Look at these existing files for the correct pattern:
- `test/tap/tests/unit/connection_pool_unit-t.cpp` — unit test using harness
- `test/tap/tests/unit/rule_matching_unit-t.cpp` — unit test using harness
- `include/ConnectionPoolDecision.h` — standalone extracted function header with include guard
Refactoring Challenges
- The current implementation uses `MySrvC` objects directly, which contain connection pool pointers. The extraction must use a lightweight struct (like `ServerCandidate` above) with only the decision-relevant fields.
- GTID-based filtering is MySQL-specific and adds complexity — it can be excluded from the initial extraction.
- The actual `get_random_MySrvC()` also handles GTID constraints — those can be a separate extraction.
Acceptance Criteria
- Extracted function uses a simple struct (no MySrvC dependency)
- Existing E2E TAP tests pass unchanged
- Weighted distribution verified statistically
- All server status types tested
- Edge cases (empty hostgroup, all offline, max_connections) tested
- PR targets `v3.0-5473` (NOT `v3.0`)
- Test file is in `test/tap/tests/unit/`
- Test uses harness (`test_globals.h`, `test_init.h`)
- Test does NOT reimplement extracted functions
- Doxygen documentation on extracted function