Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.62%. Comparing base (7cae7b7) to head (e3dc3d0).
⚠️ Report is 1 commits behind head on main.

@@             Coverage Diff              @@
##               main    #4450      +/-   ##
============================================
+ Coverage     64.88%   65.62%   +0.73%     
============================================
  Files          1649     1612      -37     
  Lines        128345   124322    -4023     
  Branches      27525    26910     -615     
============================================
- Hits          83282    81583    -1699     
+ Misses        33335    31228    -2107     
+ Partials      11728    11511     -217     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code Review

Good addition overall - the parameterized approach across three protocols, the well-documented async assertion strategy, and the reuse of ContainersTestTemplate infrastructure are all solid. A few issues to address before merging.

BUGS / CORRECTNESS

Resource leaks when assertions fail

In SingleServerTimeSeriesLoadTestIT, admin.close() is called at the end of the test body but not in a finally block. If any assertion fails (e.g. assertAggregateExtremes), the remote connection is leaked. Same problem in ThreeNodesTimeSeriesLoadTestIT for ingest, r0, r1, r2. Implement AutoCloseable on TimeSeriesDatabaseWrapper and use try-with-resources, or wrap in a try/finally.

MISSING @tag("slow") ANNOTATIONS

From CLAUDE.md: "Use @tag("slow") for functional regression tests that take noticeably long (large batches, multi-second elapsed time, big payloads)."

Both test classes ingest 50,000+ points, poll for minutes, and the 3-node test requires ~10 GB of Docker resources. They need @tag("slow") plus import org.junit.jupiter.api.Tag on both test classes.

SILENT EXCEPTION SWALLOWING IN assertThatPointCountIs

The existing DatabaseWrapper.assertThatUserCountIs tracks lastException and propagates it with an AssertionError if the deadline expires without a successful count. The new implementation discards all exceptions silently. If the connection is broken throughout the 30s window, the test will report "-1 != expectedTotal" with no indication of why queries kept failing. Mirror the existing pattern.

STYLE / CONVENTION ISSUES

Inconsistent API style in ThreeNodesTimeSeriesLoadTestIT

The lambda body uses servers.getFirst() (Java 21 SequencedCollection API), but directly above in the same method, the outer code uses servers.get(0). The existing SingleServerLoadTestIT uses getFirst(), so standardise on that.

Float equality comparison in assertAggregateExtremes

The check agg.min() == expectedMin && agg.max() == expectedMax uses ==. 15.0 and 34.0 happen to be exactly representable in IEEE 754 so this works today, but isCloseTo(expectedMin, offset(1e-9)) is safer and signals intent more clearly for any future reader.

URL parameter not encoded in latestTimestamp

The tag value (e.g. "sensor_id:verify") contains a colon and is appended raw to the URL query string. Use URLEncoder.encode(tag, StandardCharsets.UTF_8) to be safe against future tag values that contain characters requiring percent-encoding.

SQL protocol batching asymmetry

Line protocol batches 500 points per HTTP call; SQL protocol sends one INSERT per round-trip (10,000 per thread). The three protocol cases therefore measure fundamentally different throughput characteristics and the SQL cases will be considerably slower. Consider batching SQL inserts via sqlscript, or add a Javadoc note so readers know the timings are not comparable across protocols.

MINOR

• ResultSet returned by countPoints(), aggregate(), and checkSchema() is never closed (consistent with the pre-existing DatabaseWrapper pattern, but worth a follow-up issue to track).
• The nested polling in assertAggregateExtremes (outer loop up to 60s, inner aggregate() up to 30s each call) can compound to 90s per assertion since aggregate() already retries internally. Consider collapsing into one polling level to keep the total bounded.

SUMMARY

Code Review

Good addition - the time series load tests fill a real gap and the three-protocol parameterisation (LINE_PROTOCOL, SQL_HTTP, SQL_GRPC) gives broad coverage. The TimeSeriesDatabaseWrapper is well-structured and the explanatory comments on time series query semantics are valuable. A few issues to address before merging:

Required fixes

1. Missing @Tag("slow") on both IT classes

CLAUDE.md explicitly requires @Tag("slow") on functional tests that take noticeably long. Both classes ingest 50 000+ points in Docker containers and take minutes to run - they must be tagged.

// SingleServerTimeSeriesLoadTestIT.java
@Tag("slow")
class SingleServerTimeSeriesLoadTestIT extends ContainersTestTemplate { …

// ThreeNodesTimeSeriesLoadTestIT.java
@Tag("slow")
class ThreeNodesTimeSeriesLoadTestIT extends ContainersTestTemplate { …

2. URL encoding missing in latestTimestamp()

TimeSeriesDatabaseWrapper.java appends the tag value directly to the URL:

url.append("&tag=").append(tag);

The values passed from the tests include a colon (sensor_id:sensor-0). While the colon and dash happen to work with the current server implementation, query-string values should be percent-encoded. If a future tag or sensor name includes +, &, =, %, or a space, the request will silently send the wrong value. Use URLEncoder.encode(tag, StandardCharsets.UTF_8).

3. Interrupt handling in the monitoring loop does not break

Both SingleServerTimeSeriesLoadTestIT and ThreeNodesTimeSeriesLoadTestIT have the same pattern:

} catch (final InterruptedException e) {
    Thread.currentThread().interrupt();
    // no break!
}

Re-setting the interrupt flag without breaking means the loop re-enters, calls logger.info, then attempts sleep again, which immediately throws InterruptedException again - an effective spin-loop until the executor terminates. Compare DatabaseWrapper.createDatabase() which breaks on interrupt. Add break; after Thread.currentThread().interrupt().

4. Interrupt handling in createSchema() and createDatabase() differs from the existing helper

DatabaseWrapper.createDatabase() explicitly interrupts and breaks on InterruptedException in the retry loop. TimeSeriesDatabaseWrapper.createDatabase() delegates to the private sleep() helper which re-interrupts but doesn't signal the loop to stop, so after an interrupt the loop keeps retrying. The createSchema() method has the same issue. For consistency and correctness, the retry loops should break on interrupt.

Minor / suggestions

5. @Disabled regression test has no issue reference

TimeSeriesConcurrentInsertTest is a valuable document of a known bug, but with no linked issue it can get stranded indefinitely. Add the tracking issue number to the @Disabled reason string (e.g. "Tracks issue #NNNN"), or raise an issue for the append-path fix if one doesn't exist yet.

6. ThreeNodesTimeSeriesLoadTestIT.tearDown() comment is misleading

The comment says "Skip compareAllDatabases()" but ContainersTestTemplate.tearDown() does not call compareAllDatabases() by default - it never did. The override is an exact delegation to super.tearDown(), which means it's a no-op override (copied from ThreeNodesLoadTestIT which has the same pattern). Either remove the override entirely, or add a comment explaining why this class needs to suppress a future base-class addition.

7. aggregate() may sleep past the deadline

final long deadline = System.currentTimeMillis() + 30_000;
while (true) {
    final ResultSet rs = db.query("sql", sql, sensorId);
    if (rs.hasNext()) { … return; }
    if (System.currentTimeMillis() >= deadline)
        throw new IllegalStateException(…);
    sleep(1_000);           // no second deadline check before sleeping
}

If the deadline expires between the >= deadline check and the sleep(1_000) call, the method overshoots by up to one extra second. Not a real problem here, but assertAggregateExtremes then polls aggregate in its own loop, so the worst-case overshoot can compound. Consider checking the deadline again before sleeping.

8. insertSql() silently swallows exceptions

Insertion failures in the SQL path are only logged and counted; they don't propagate. This means the final assertThatPointCountIs can fail with a confusing count mismatch rather than the root-cause exception. Consider logging the full stack trace (not just e.getMessage()) so failures are diagnosable in CI output.

9. SQL path does not batch

ingestSeries() issues one INSERT per point when protocol != LINE_PROTOCOL (10 000 per thread). The line-protocol path batches in groups of 500. This is presumably intentional (stress-testing single-row throughput), but worth confirming - if the intent is throughput parity, SQL batching with sqlscript or multi-value INSERT would be needed.

Nit

TimeSeriesDatabaseWrapper.java field alignment uses wide padding (public static final String TYPE_NAME) which is unusual for this codebase. Minor, but worth aligning with the surrounding style.

Addressed in e3dc3d0. Per item:

• Resource leaks on assertion failure - Fixed. TimeSeriesDatabaseWrapper now implements AutoCloseable and both ITs use try-with-resources for the top-level wrappers (admin; ingest/r0/r1/r2), so connections close even when an assertion throws.
• @Tag("slow") - Not added, deliberately. The load-tests module has zero @Tag usage: these are *IT classes already excluded from the default build (failsafe, skipITs=true by default), which is why the sibling SingleServerLoadTestIT/ThreeNodesLoadTestIT are untagged. CLAUDE.md's @Tag("slow") targets slow tests in the fast (surefire) build; these ITs are already gated, so tagging adds nothing here. (For contrast, the engine-module regression test added in this PR is @Tag("slow"), because it does run in the fast build.)
• Silent exception swallowing in assertThatPointCountIs - Fixed. It now tracks the last exception and throws an AssertionError carrying it, mirroring DatabaseWrapper.assertThatUserCountIs.
• getFirst() vs get(0) - Fixed. Standardised on servers.getFirst() for index 0.
• Float == - Fixed. assertAggregateExtremes now uses isCloseTo(expected, within(1e-9)).
• URL-encode the tag - Fixed. latestTimestamp now URLEncoder.encode(tag, UTF_8).
• SQL vs line-protocol batching asymmetry - Added a Javadoc note on ingestSeries that the protocols are intentionally not throughput-comparable. The per-row SQL path is kept on purpose (not batched into sqlscript): it exercises the single-row append path under concurrency, which is what the SQL cases are meant to stress.
• Nested polling could compound to 90s - Fixed. aggregate() is now a single query (returns null when no rows are visible yet) and assertAggregateExtremes owns the single 60s polling loop, so there is no longer a retry-within-retry.
• ResultSet not closed (minor) - Not changing. RemoteDatabase.query/command return an in-memory InternalResultSet materialised eagerly from the response; its close() only does content.clear(), so no server cursor or connection is held (and this matches the sibling DatabaseWrapper). No leak to track.

Note: the SQL-protocol cases currently fail at POINTS_PER_THREAD = 10000 on an exact-count assertion. That is a separate, confirmed server-side data-loss under highly concurrent single-row time series INSERTs (sealed-slot lost update; line protocol and single-threaded are unaffected) - under investigation separately, not introduced by these tests. A @Disabled engine regression test for it is included in this PR.

Code Review

This PR adds solid Testcontainers-based load tests for the time series engine. The structure is clean and follows the existing ContainersTestTemplate pattern well. A few issues worth addressing:

Correctness / Bugs

1. Interrupt handling in retry loops (TimeSeriesDatabaseWrapper)

createDatabase() and createSchema() both use the sleep(long) helper, which catches InterruptedException and restores the thread's interrupt flag but does not break the retry loop. If the calling thread is interrupted during a retry, the loop silently continues for up to maxAttempts * 2s longer. The existing DatabaseWrapper.createDatabase() is consistent about this: it explicitly breaks after restoring the flag. The new wrapper should do the same:

// createDatabase() retry sleep
try {
    Thread.sleep(2_000);
} catch (final InterruptedException ie) {
    Thread.currentThread().interrupt();
    break;
}

Same fix applies to createSchema().

2. Silent error swallowing in insertSql() and postLineProtocol()

Both methods catch any Exception, log it, increment an error counter, and return normally. If every insert for a thread fails (e.g., the server is unreachable), ingestSeries() finishes without throwing, and the subsequent assertThatPointCountIs() fails - but with a confusing count-mismatch message rather than the root cause. Consider re-throwing the exception (or at least throwing an AssertionError) after a threshold of consecutive failures, so test failures are easier to diagnose.

3. latestTimestamp() - no bounds check on the latest array

final JSONArray latest = json.getJSONArray("latest");
return latest.getLong(0);

If the server returns "latest":[] (e.g., the verification series hasn't replicated yet), this throws JSONException without any context. A guard like assertThat(latest.length()).isGreaterThan(0) before the access would give a clearer failure message.

Design / Maintainability

4. ThreeNodesTimeSeriesLoadTestIT.tearDown() override

The override body is just super.tearDown(), which is exactly what would happen without the override. The comment explains the intent ("we're not calling compareAllDatabases()") but the base class ContainersTestTemplate.tearDown() doesn't call compareAllDatabases() either - only ThreeNodesLoadTestIT does. The override and its comment are correct as documentation (mirroring the pattern in ThreeNodesLoadTestIT), but it would be clearer to add a brief note that this intentionally matches the sibling class's override.

5. Executor wait loop has no upper-bound timeout

Both load tests use while (!executor.isTerminated()) without a ceiling. If a worker thread hangs (e.g., waiting on a server that stopped responding), the test will never complete. executor.awaitTermination(10, TimeUnit.MINUTES) followed by executor.shutdownNow() and an assertion would make the hang self-diagnosing. This pattern is inherited from SingleServerLoadTestIT, so it's a pre-existing issue, but worth fixing here since the new tests have a heavier footprint.

6. assertAggregateExtremes() - manual polling vs Awaitility

The method implements its own poll loop with System.currentTimeMillis(), while ThreeNodesTimeSeriesLoadTestIT correctly uses Awaitility for convergence checking. Replacing the manual loop with:

Awaitility.await()
    .atMost(60, TimeUnit.SECONDS)
    .pollInterval(2, TimeUnit.SECONDS)
    .untilAsserted(() -> {
        final AggResult agg = aggregate(sensorId);
        assertThat(agg).isNotNull();
        assertThat(agg.min()).isCloseTo(expectedMin, within(tolerance));
        assertThat(agg.max()).isCloseTo(expectedMax, within(tolerance));
    });

would unify the polling style and provide better failure messages on timeout.

7. Magic number 4L in expected total

bulkPoints + 4L appears in both test classes. The 4L comes from ingestVerificationSeries() (four hardcoded points). Naming it:

private static final int VERIFICATION_POINTS = 4;
...
final long expectedTotal = bulkPoints + VERIFICATION_POINTS;

or sharing the constant from TimeSeriesDatabaseWrapper would make the intent clearer without relying solely on the comment.

The @Disabled concurrent-insert regression test

TimeSeriesConcurrentInsertTest.concurrentSingleRowInsertsDoNotLoseSamples() is a well-written reproduction of a real bug with clear documentation of when it should be enabled. One concern: being @Disabled, it will never run in CI and may be forgotten. Consider opening a tracking issue and linking it from the @Disabled comment (e.g., @Disabled("see issue #XXXX - ...")), so the bug and its test stay connected.

Minor

• basicAuth() is called on every HTTP request and recomputes the Base64 encoding each time. Precomputing it once in the constructor is a trivial win.
• checkSchema() queries count(*) just to check the type exists; a SELECT 1 FROM sensor LIMIT 1 would be cleaner and avoids a full scan on large datasets (though irrelevant for test startup).
• countPoints() and checkSchema() don't close the ResultSet. Depending on RemoteDatabase's implementation this may be a resource leak; wrapping in try-with-resources would make it explicit.

Overall this is a well-thought-out addition that exercises realistic ingestion patterns across three protocols and two topologies. The documented semantics around sealed vs. mutable data and the rationale for polling assertions are particularly helpful. The issues above are mostly defensive coding concerns rather than correctness blockers.