Copilot commented on code in PR #434:
URL: https://github.com/apache/fluss-rust/pull/434#discussion_r2901178773
##########
bindings/cpp/test/test_utils.h:
##########
@@ -291,7 +316,8 @@ class FlussTestEnvironment : public ::testing::Environment {
GTEST_SKIP() << "Fluss cluster did not become ready within timeout.";
}
- void TearDown() override { cluster_.Stop(); }
+ // Cluster stays alive for parallel processes and subsequent runs.
+ void TearDown() override {}
Review Comment:
`TearDown()` no longer stops the Docker cluster, which means running
`fluss_cpp_test` directly (outside of CTest fixtures) will leave
containers/network running after the process exits. If the intent is to keep
the cluster only for `ctest -j` runs, consider making this conditional (e.g.,
keep-alive only when a specific env var/flag is set) and otherwise call
`cluster_.Stop()` to avoid resource leaks and surprising local state.
```suggestion
// Cluster stays alive for parallel processes and subsequent runs if
explicitly requested.
void TearDown() override {
// Allow keeping the cluster alive only when
FLUSS_CPP_TEST_KEEP_ALIVE=1 is set.
const char* keep_alive = std::getenv("FLUSS_CPP_TEST_KEEP_ALIVE");
if (!keep_alive || std::strcmp(keep_alive, "1") != 0) {
cluster_.Stop();
}
}
```
##########
bindings/cpp/test/test_utils.h:
##########
@@ -126,29 +126,42 @@ class FlussTestCluster {
const char* env_servers = std::getenv("FLUSS_BOOTSTRAP_SERVERS");
if (env_servers && std::strlen(env_servers) > 0) {
bootstrap_servers_ = env_servers;
+ const char* env_sasl = std::getenv("FLUSS_SASL_BOOTSTRAP_SERVERS");
+ if (env_sasl && std::strlen(env_sasl) > 0) {
+ sasl_bootstrap_servers_ = env_sasl;
Review Comment:
When FLUSS_BOOTSTRAP_SERVERS is set but FLUSS_SASL_BOOTSTRAP_SERVERS is not,
`sasl_bootstrap_servers_` remains empty. SASL tests (e.g. `test_sasl_auth.cpp`)
will then try to connect with an empty bootstrap server string and fail.
Consider defaulting `sasl_bootstrap_servers_` to `bootstrap_servers_` (or
skipping SASL tests with a clear message) unless FLUSS_SASL_BOOTSTRAP_SERVERS
is provided.
```suggestion
sasl_bootstrap_servers_ = env_sasl;
} else {
// Default SASL bootstrap servers to the plain bootstrap
servers
sasl_bootstrap_servers_ = bootstrap_servers_;
```
##########
bindings/cpp/test/test_utils.h:
##########
@@ -126,29 +126,42 @@ class FlussTestCluster {
const char* env_servers = std::getenv("FLUSS_BOOTSTRAP_SERVERS");
if (env_servers && std::strlen(env_servers) > 0) {
bootstrap_servers_ = env_servers;
+ const char* env_sasl = std::getenv("FLUSS_SASL_BOOTSTRAP_SERVERS");
+ if (env_sasl && std::strlen(env_sasl) > 0) {
+ sasl_bootstrap_servers_ = env_sasl;
+ }
external_cluster_ = true;
std::cout << "Using external cluster: " << bootstrap_servers_ <<
std::endl;
return true;
}
+ // Reuse cluster started by another parallel test process or previous
run.
+ if (WaitForPort("127.0.0.1", kPlainClientPort, /*timeout_seconds=*/1))
{
+ SetBootstrapServers();
+ external_cluster_ = true;
+ return true;
+ }
Review Comment:
The fast-path for reusing an existing cluster only checks `kPlainClientPort`
for 1s and then returns success. That can mark the cluster as reusable while
other required listeners (tablet/coordinator SASL, tablet plaintext) are still
starting, which can make parallel tests flaky. Consider reusing
`WaitForCluster()` here (or checking all required ports) before returning.
--
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]
