PDGGK commented on PR #110:
URL: https://github.com/apache/iotdb-extras/pull/110#issuecomment-4785285762
Thanks for the very detailed review — the version-layer findings in
particular were spot on. Here is what I changed, point by point. Verified
locally with a cache-off full build: **79 unit tests + 9 Testcontainers ITs
(against `apache/iotdb:2.0.8-standalone`)**, plus apache-rat, checkstyle and
spotless all clean.
## 🔴 1 — version overrides / reactor convergence
I removed **all** of the iotdb / tsfile / guava overrides. The module now
inherits the parent reactor's `iotdb-session 2.0.5`, `tsfile 2.1.1` and `guava
32.1.2-jre` — exactly like the sibling spark/flink connectors — so there is a
**single tsfile version across the reactor** and the convergence risk you
flagged is gone (confirmed via `dependency:tree`).
The only code change this required was switching the builder call back to
`TableSessionPoolBuilder.enableCompression(...)` (the 2.0.5 method name). I
verified the full write path still works at runtime: the 9 container ITs
(including a 500-point mixed-type single-flush) pass against the
`apache/iotdb:2.0.8-standalone` server, so the 2.0.5 client / 2.0.8 server RPC
pairing is fine.
I deliberately did **not** keep `iotdb-session 2.0.8` and only drop tsfile:
I tested that combination and `iotdb-session 2.0.8` was compiled against tsfile
2.2.1 and calls `TSFileConfig.getValueEncoder(...)` at runtime, which does not
exist in tsfile 2.1.1 — a large flush fails with `NoSuchMethodError`. Keeping
2.0.8 would therefore force a tsfile override again, i.e. reintroduce exactly
the divergence you raised. Inheriting 2.0.5/2.1.1 is the clean resolution.
One honest note on `-P enforce`: after this change the tsfile conflict is
gone, but `dependencyConvergence` still reports one **pre-existing** finding on
`org.apache.httpcomponents:httpcore` (4.4.12 vs 4.4.16). It comes transitively
through `iotdb-session 2.0.5 → libthrift 0.14.1` and is shared by **every**
iotdb-session consumer in the reactor (spark, flink, spring-boot-starter,
collector, examples) — the parent pins `httpclient` but not `httpcore`. It is
not introduced by this module and is best resolved at the parent level; I'm
happy to file a separate small PR adding `httpcore` to the parent
`dependencyManagement` if you'd like.
## 🔴 2 — jakarta.validation-api 3.0.2
I kept 3.0.2 and took the "justify the divergence" path you offered. The
deployment host is **ThingsBoard 4.3.1.2, which runs on Spring Boot 3.5.14 /
Spring 6 / JDK 17** (from its own pom) — i.e. the `jakarta.*` namespace. The
module is compiled to deploy into that host, so `jakarta.validation.*` (3.0.2)
is the correct namespace; downgrading to `javax.validation` 2.0.2 would break
validation at the real runtime. The annotations are `provided`-scope and the
Hibernate Validator provider is supplied by the ThingsBoard runtime, not
bundled here. I made the override explicit at the dependency and documented the
rationale in the pom.
You implicitly surfaced a real gap here, which I also fixed: the previous
tests only exercised a hand-built validator. I added a test that drives
Spring's real binding/validation path (`Binder` + `ValidationBindHandler`) and
asserts a bad bound value is rejected with a `BindValidationException`, with a
paired positive case so the rejection is attributable to the constraint, not
the wiring.
## 🔴 3 — explicit named profile
Done exactly as your inline comments: the root-pom profile is now
`with-thingsboard`, the `<jdk>[17,)</jdk>` activation is removed, and CI
activates it on the JDK 17+ jobs via `-P with-thingsboard` in
`compile-check.yml` (the 8/11 jobs omit the flag and skip it). This mirrors
`with-springboot` / `iotdb-spring-boot-starter`.
One consequence worth flagging: now that it is no longer JDK-auto-activated,
the module is built only by workflows that pass `-P with-thingsboard`. I added
that to `compile-check.yml` (build + tests on 17/21), but `code-analysis.yml`
(CodeQL autobuild) and `dependency-check.yml` (SBOM) don't pass connector
profiles, so this module is no longer included there — which matches the other
opt-in modules (`with-springboot`, `with-examples`). Happy to add `-P
with-thingsboard` to those workflows if you'd prefer to keep this module under
CodeQL/SBOM coverage; I left them as-is to stay consistent with the existing
convention.
## 🟠 4 — retry classification
`insertWithRetry` now also catches `StatementExecutionException` and retries
**only transient** server-side conditions, keyed on `TSStatusCode`
(`WRITE_PROCESS_REJECT`, `INTERNAL_REQUEST_TIME_OUT`,
`INTERNAL_REQUEST_RETRY_ERROR`, `TOO_MANY_CONCURRENT_QUERIES_ERROR`,
`DISPATCH_ERROR`). Permanent failures (parse / type / schema) fail fast. Added
two tests asserting the exact `insert` invocation counts for a transient vs a
permanent code.
## 🟠 5 — schema idempotency
The DDL now uses `CREATE TABLE IF NOT EXISTS` (valid on 2.0.8 Table Mode —
exercised by the IT), so the normal re-run returns SUCCESS without throwing. As
defense-in-depth, `isAlreadyExists` now checks the structured status code
(`TABLE_ALREADY_EXISTS` / `DATABASE_ALREADY_EXISTS` / `COLUMN_ALREADY_EXISTS`)
over the cause chain, with the message-substring match kept only as a
last-resort fallback. Added a test proving the code-driven path (an
already-exists code with a non-matching message is still tolerated).
## 🟠 6 — no-ThingsBoard startup test
Added a context-startup test that boots the whole
`@Import(IoTDBTableConfiguration.class)` chain under a
`FilteredClassLoader("org.thingsboard")` **with the selector enabled**, and
asserts the context starts cleanly (`hasNotFailed()`), creates no module beans,
and that the ThingsBoard class is genuinely absent (`ClassNotFoundException`).
I also strengthened the existing discovery test with a positive
`hasNotFailed()` assertion. (It's a context-slice test rather than a full
`@SpringBootTest`, but it drives the real activation path with the class
actually hidden.)
## 🟠 7 — dual registration / Boot version
ThingsBoard's actual Spring Boot version is **3.5.14** (from TB 4.3.1.2's
pom), so `@AutoConfiguration` + `AutoConfiguration.imports` is the **active and
correct** mechanism. I kept the `spring.factories` `EnableAutoConfiguration`
entry only for Boot-2.7 portability (it is ignored on Boot 3.x) and corrected
the javadoc/README to state this clearly rather than calling it a vague
"fallback".
## 🟡 minor
- `dataPointDays` now saturates at `Integer.MAX_VALUE` instead of
`Math.toIntExact` throwing.
- `flushThreads` keeps the exact-1 constraint (so an unsupported value fails
fast) but now carries a clear custom validation message + javadoc, instead of a
bare `@Min(1)@Max(1)`.
- `guava` override dropped (inherits the parent). `mockito` override kept
(this module is JUnit 5 / JDK 17 only and needs mockito 5.x) with a comment
explaining the divergence from the parent's PowerMock-pinned 2.x.
- `defaultTtlMs` log key aligned to the field name.
- `CI-NOTES.md` updated (version policy + the pre-existing httpcore note).
- CodeQL: removed the useless null check. The "start of thread in
constructor" note is a deliberate, safe design — all worker-visible fields are
final and assigned before the thread starts, and the package-private
`startWorker` flag keeps construction and start separable (the unit tests
construct the writer without starting it); the class is not intended for
subclassing, so I kept it as-is. The "useless parameter" alerts are on the
`src/provided` SPI stubs, whose signatures must mirror the upstream ThingsBoard
interfaces, so those stay too.
Thanks again — happy to iterate on any of these.
--
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]
