Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Java HBase 1.x/2.x semantic-convention conformance backed by locally built, checksum-verified fixtures.
Changes:
- Adds HBase Get, Put, Scan, and Batch scenarios.
- Adds HBase fixture lifecycle and span classification.
- Pins supported client versions and generated coverage.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.github/renovate.json5 |
Constrains HBase updates. |
scenarios/database/java/README.md |
Documents HBase coverage. |
scenarios/database/java/gradle/libs.versions.toml |
Pins HBase clients. |
scenarios/database/java/settings.gradle.kts |
Registers HBase projects. |
scenarios/database/java/shared/hbase/scenarios/build.gradle.kts |
Configures shared workload. |
scenarios/database/java/shared/hbase/scenarios/src/main/java/io/opentelemetry/conformance/database/hbase/HbaseScenario.java |
Implements HBase operations. |
scenarios/database/java/hbase/hbase-1/opentelemetry-javaagent/build.gradle.kts |
Configures HBase 1 launcher. |
scenarios/database/java/hbase/hbase-1/opentelemetry-javaagent/conformance.yaml |
Defines HBase 1 expectations. |
scenarios/database/java/hbase/hbase-1/opentelemetry-javaagent/data.json |
Records HBase 1 coverage. |
scenarios/database/java/hbase/hbase-1/opentelemetry-javaagent/database.yaml |
Selects HBase 1 backend. |
scenarios/database/java/hbase/hbase-2/opentelemetry-javaagent/build.gradle.kts |
Configures HBase 2 launcher. |
scenarios/database/java/hbase/hbase-2/opentelemetry-javaagent/conformance.yaml |
Defines HBase 2 expectations. |
scenarios/database/java/hbase/hbase-2/opentelemetry-javaagent/data.json |
Records HBase 2 coverage. |
scenarios/database/java/hbase/hbase-2/opentelemetry-javaagent/database.yaml |
Selects HBase 2 backend. |
tools/database/runner/README.md |
Documents HBase fixtures. |
tools/database/runner/src/database_conformance/__init__.py |
Registers HBase backends. |
tools/database/runner/src/database_conformance/_container.py |
Adds fixed ports and diagnostics. |
tools/database/runner/src/database_conformance/_coverage.py |
Classifies HBase spans. |
tools/database/runner/src/database_conformance/_hbase.py |
Implements HBase fixtures. |
tools/database/runner/src/database_conformance/hbase-image/Dockerfile |
Builds pinned HBase images. |
tools/database/runner/src/database_conformance/hbase-image/hbase-site.xml |
Configures local HBase. |
tools/database/runner/src/database_conformance/hbase.rb |
Initializes test data. |
tools/database/runner/tests/test_database_backends.py |
Tests fixture lifecycle. |
tools/database/runner/tests/test_database_coverage.py |
Tests HBase classification. |
tools/database/runner/tests/test_database_model.py |
Validates registry support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tools/database/runner/src/database_conformance/_hbase.py:46
DockerImage.remove()only deletes the image when itsclean_upflag is true. Withclean_up=False, both cleanup calls below merely close the Docker client, so every HBase fixture leaves its tagged image behind despite the lifecycle and tests expecting removal. Enable cleanup here (manualremove()calls will still control when deletion occurs).
clean_up=False,
Cover HBase 1.x and 2.x client instrumentation with version-matched Apache fixtures and deterministic database expectations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot comment: The surrounding handler catches `BaseException`, so this new wrapping also converts `KeyboardInterrupt` and `SystemExit` into `DatabaseBackendError`. The previous bare `raise` preserved those control-flow exceptions after cleanup; retain that behavior so an interrupted startup remains interruptible. Copilot comment: `get_logs()` is not guaranteed to raise `DockerException`: when `container.start()` fails before Testcontainers has created a wrapped container, `get_logs()` raises a runtime "not started" error. That exception then escapes `_failure_message`, masks the real startup failure, and skips `close()`. Make this best-effort diagnostic helper catch ordinary exceptions so the original failure is still reported and cleanup still runs. Analysis: The handler now builds diagnostics only for ordinary exceptions and re-raises control-flow exceptions after cleanup. Log collection catches ordinary exceptions because it is best-effort and must not replace the startup failure. Upsides: Interrupted startup remains interruptible. An unavailable log stream no longer masks the startup failure or skips cleanup. Downsides: Log collection reports ordinary helper errors as diagnostic text instead of propagating them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment:
`DockerImage.remove()` only deletes the image when its `clean_up` flag is true. With `clean_up=False`, both cleanup calls below merely close the Docker client, so every HBase fixture leaves its tagged image behind despite the lifecycle and tests expecting removal. Enable cleanup here (manual `remove()` calls will still control when deletion occurs).
```
clean_up=False,
```
Analysis: `DockerImage.remove()` checks the `clean_up` setting before it removes the built image. Enabling that setting lets the existing explicit cleanup calls delete each temporary HBase image.
Upsides: Successful and failed HBase fixture runs no longer leave tagged images behind. The test now verifies the cleanup setting passed to Testcontainers.
Downsides: Each fixture run must rebuild its HBase image after cleanup.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
trask
force-pushed
the
trask-hbase-conformance
branch
from
August 30, 2026 22:59
c6ca0c0 to
743209b
Compare
…le version constants
Review finding:
HBASE_1_IMAGE and HBASE_2_IMAGE are dead in this module. HBase.__init__ recomputes the tag as f"otel-conformance-hbase:{version}" and never reads either constant, so nothing in database_conformance uses them; only tools/database/runner/tests/test_database_backends.py imports them. The sibling backends do the opposite: MARIADB_IMAGE and POSTGRES_IMAGE are the single definition of the image and their BackendSpec reads them. Here the tag format and the version string are stated twice, so editing HBASE_1_IMAGE alone would change nothing the runner does. Fix: let HBase1 and HBase2 pass the image tag (for example super().__init__(HBASE_1_IMAGE, "1.7.2", _HBASE_1_SHA512), or derive the constants and the tag from one version constant) so the constants are the one place the fixture tag is defined.
Analysis: The module stated each fixture version twice, once in the exported image constant and once in the literal HBase1 or HBase2 passed to HBase.__init__, and stated the tag format twice as well. Only the second copy reached the Docker build, so the exported constants described the fixture without configuring it. HBASE_1_VERSION and HBASE_2_VERSION now hold each version once, _image_name builds the tag, and the exported constants are derived from both. HBase1 and HBase2 pass the version constants, so the tag the runner builds and the tag the constant names cannot disagree.
Upsides: One definition per fixture version and one definition of the tag format, so a version bump changes a single line and the exported constant follows it. The constants now describe what the runner actually builds, which is what the existing tests assert. Matches how MARIADB_IMAGE and POSTGRES_IMAGE relate to their backend specs.
Downsides: Adds a small module-level helper and orders the module so the image constants follow it, which is slightly more indirection than two string literals.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review finding:
This line updates the Java README to name HBase, but the parent domain README, scenarios/database/README.md, still describes the domain as PostgreSQL and MariaDB only and is now wrong. It says "Initial support is Java-only and exercises PostgreSQL and MariaDB through JDBC.", its directory map lists only java/shared/jdbc/... and java/{postgresql,mariadb}/jdbc/, its scenario table maps every scenario to a JDBC path, and its "Running it" section lists only the four JDBC packages. After this PR the domain also ships scenarios/database/java/hbase/hbase-1 and hbase-2 and shared/hbase/scenarios, and their scenarios are HBase client operations, not JDBC paths. Fix: update scenarios/database/README.md the same way this line updates the Java README - mention HBase beside PostgreSQL and MariaDB, add the HBase directories to the layout block, add the HBase scenario-to-operation rows, and list the two HBase packages under "Running it".
Review finding:
This added sentence documents that HBase advertises fixed ZooKeeper, master, and region-server ports, but the runner-variable table a few lines below still describes DATABASE_PORT as "Docker-assigned host port". For the new hbase-1 and hbase-2 backends that is false: BackendSpec.fixed_ports makes _container.py bind 2181, 16000, and 16020 to the same numbers on the host, so DATABASE_PORT is always 2181 rather than an ephemeral port Docker picked. A package author reading the table would believe the port is arbitrary per run. Fix: reword the DATABASE_PORT row so it covers both cases, for example "Host port published by Docker; ephemeral for PostgreSQL and MariaDB, fixed for HBase".
Analysis: Both findings are the same omission seen in two files: the HBase backends changed what these documents describe, and neither document changed with them. The domain README now names HBase in its opening paragraph, lists the shared HBase workload and the two HBase packages in its layout block, maps the four HBase scenarios to the client calls they make, runs both HBase packages in its command list, points at the hbase.rb schema beside the two SQL schemas, and says that the fixed loopback ports let only one HBase package run at a time. The runner README's DATABASE_PORT row now separates the Docker-assigned port the SQL backends publish from the fixed 2181 the HBase backends publish, which is what BackendSpec.fixed_ports produces.
Upsides: A reader who starts at the domain README finds every package the domain ships and the operation each scenario measures. A package author reading the runner-variable table learns that DATABASE_PORT is fixed for HBase before writing a conformance.yaml that assumes otherwise.
Downsides: The domain README now carries two scenario tables and grows with each backend that is not JDBC. No material downside identified beyond that.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review finding: The comment claims these rules keep each representative client "aligned with the matching fixture version", but the rules cannot do that. allowedVersions ">=1.7.2 <2.0.0" and ">=2.4.18 <2.5.0" only keep the Gradle hbase-client inside its API line, so Renovate is free to raise scenarios/database/java/gradle/libs.versions.toml to, say, 1.7.3 or 2.4.19. The server fixture version lives in tools/database/runner/src/database_conformance/_hbase.py as the literals passed by HBase1 and HBase2, together with a SHA512 that only matches that exact distribution, and nothing updates or checks it: the customManagers block in this same file covers _postgres.py and _mariadb.py but not _hbase.py. So the client and fixture silently drift apart and the comment states an invariant nothing enforces. Fix: make the comment say only what the rules do (keep each client inside the API line its fixture serves) and note that the fixture version and checksum in _hbase.py must be updated by hand alongside a client bump, or narrow allowedVersions to the exact fixture versions so a bump cannot land without touching the fixture. Analysis: The rules constrain only the Gradle hbase-client version range, so a reader who trusted the comment would expect Renovate to move the local server fixture too. The comment now describes the range as the API range the fixture serves, and names the file that carries the fixture version and its checksum, so whoever reviews a client bump knows the fixture does not follow on its own. Narrowing allowedVersions to the exact fixture versions was the alternative, and it is not taken: a patch-level client inside the same API line still works against the fixture, so exact pinning would block routine updates without evidence that the drift breaks anything. Upsides: The comment states only what the configuration does, and it points at the one file a maintainer has to touch when a client bump should move the fixture with it. Downsides: The comment is longer, and it still relies on a maintainer reading it, since nothing checks that the client and fixture versions agree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check: Scenarios (database, java): expected metrics ['otlp.exporter.exported', 'processedLogs'] but they were not emitted Cause: HBase client logs activate Java SDK telemetry counters whose export timing is not guaranteed before the scenario process exits. Fix: Disable log export for HBase scenarios and stop requiring the resulting SDK-internal counters, while keeping the database duration metric exact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: This fixture binds ZooKeeper to host port 2181, so the exported `DATABASE_PORT` is 2181, not 32768. The generic stub currently hard-codes 32768 from the dynamic-port backends, which makes this assertion contradict the HBase contract and leaves the fixed-port variable behavior untested. Please make `StubContainer.get_exposed_port()` model the configured binding (or accept a published-port value) and assert `"2181"` here. Analysis: `StubContainer.get_exposed_port()` should return the published host port configured for each test. The default keeps the dynamic PostgreSQL and MariaDB behavior at 32768. The HBase fixture sets the published port to its fixed ZooKeeper port, 2181. Upsides: The HBase test now verifies the fixed-port contract, while the dynamic-port tests keep their existing coverage. Downsides: The test stub gains one constructor argument. Production behavior does not change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review finding:
This rewrite drops a documented, code-enforced constraint and leaves the replacement sentence without a subject. The removed line was "`runner_config` must contain only `backend`, set to `postgresql` or `mariadb`." The replacement is "The supported values are `hbase-1`, `hbase-2`, `mariadb`, and `postgresql`.", which never says supported values of what, and no longer states that `backend` is the only key allowed. That rule is real and enforced: `_backend_name` in tools/database/runner/src/database_conformance/__init__.py raises SpecError("... must contain exactly one string key named 'backend'") when `set(config) != {"backend"}`, and tools/database/runner/tests/test_database_session.py exercises it for an extra key, a wrong key, and an empty mapping. This README is the only prose that told a package author about it, so after this PR a package author who adds a second key under `runner_config` gets a SpecError the documentation no longer warns about. Fix: keep both facts in one sentence, for example "`runner_config` must contain only `backend`, set to `hbase-1`, `hbase-2`, `mariadb`, or `postgresql`."
Analysis: _backend_name rejects any runner_config whose key set is not exactly {"backend"}, and test_database_session.py covers an extra key, a wrong key, and an empty mapping. The runner README was the only prose stating that rule. Restoring it in the same sentence that lists the four backend values keeps both facts together and gives "the supported values" the subject it lost.
Upsides: A package author reading the runner README learns the constraint the runner enforces, instead of meeting it as a SpecError. The sentence again names what the listed values are values of.
Downsides: No material downside identified.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ession Review finding: "The HBase fixtures build cached local images" describes caching the fixture does not do. HBase.__init__ in tools/database/runner/src/database_conformance/_hbase.py constructs DockerImage(..., clean_up=True), and HBase.close() calls self._image.remove() in a finally block, so the tagged image is deleted at the end of every session. testcontainers' DockerImage.remove defaults to force=True, noprune=False, and noprune=False asks the Docker API to delete the untagged parent layers too, so the layers that held the extracted distribution go with it. Each run therefore downloads the ~250 MB Apache HBase tarball again and re-extracts it; nothing is reused between runs. A reader of this sentence would expect the download to happen once per host. Fix: say what the fixture does, for example "The HBase fixtures build local images from checksum-verified Apache HBase 1.7.2 and 2.4.18 binary distributions and a digest-pinned Eclipse Temurin base, and remove the image when the session closes." Analysis: The fixture keeps no image between sessions. DockerImage is built with clean_up=True and HBase.close() removes it in a finally block, and testcontainers removes with force=True and noprune=False, which asks Docker to drop the untagged parent layers as well. Saying "cached" invites a reader to expect the distribution download to happen once per host, so the sentence now states the lifetime instead. Upsides: The README describes the image lifetime the code implements, so nobody plans around a cache that is not there. It also makes the cost of an HBase run visible, since the distribution is fetched again each time. Downsides: No material downside identified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…xport
Review finding:
`OTEL_LOGS_EXPORTER: none` turns off a whole signal for the two new HBase packages, and it is unexplained and unique in this repository. `git grep OTEL_LOGS_EXPORTER` matches only the two new HBase conformance.yaml files; none of the four existing database packages nor any HTTP, gen-ai, or other scenario disables an exporter. The setting is not incidental, because the declared signal list is exact: `_check_names` in tools/runner/src/opentelemetry/conformance/_checks.py fails a scenario for "undeclared events emitted" as well as for missing ones, so `events: []` in scenarios/database/contracts/hbase.yaml is checked against what the run actually emits. With the logs exporter off, the agent's log appender instrumentation exports nothing, so `events: []` and the committed `"events": {}` in data.json are satisfied without the run ever producing the log signal the other packages produce. A reader of either conformance.yaml, of the contract, or of the recorded coverage cannot tell that a signal was switched off, or why. Fix: add a short comment above the line in both scenarios/database/java/hbase/hbase-1/opentelemetry-javaagent/conformance.yaml and scenarios/database/java/hbase/hbase-2/opentelemetry-javaagent/conformance.yaml saying why HBase alone needs it (the HBase client logs heavily through log4j, and the agent would export those records as undeclared events), matching how this repository comments every other non-obvious knob.
Analysis: These two packages are the only place in the repository that switches an exporter off, so the line needs to say why it is here. The reason is the one the fixture actually hit: HBase client logging starts the SDK log pipeline, whose own counters are not guaranteed to reach the collector before a short scenario process exits, which made the declared metric list unstable. The finding described the effect as undeclared events; the comment states the counter timing instead, because that is what the behavior is.
Upsides: A reader of either conformance.yaml learns why HBase alone disables a signal, so nobody removes the line and rediscovers the flake, and nobody copies it into a package that does not need it. It also matches how every other non-obvious environment switch in this repository is commented.
Downsides: The comment records a timing property of the SDK rather than a stable contract, so it can go out of date if the SDK changes when it flushes its own telemetry.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…odule
Review finding:
These two ARG defaults are a second definition of the HBase 2 pins, and the new test restates the same literal a third time instead of tying the two together. `_hbase.py` holds HBASE_2_VERSION = "2.4.18" and _HBASE_2_SHA512, and HBase.__init__ always passes both through buildargs, so the Dockerfile defaults never affect a runner build. Meanwhile `ARG HBASE_VERSION=2.4.18` and `ARG HBASE_SHA512=1d90aa46...` repeat exactly those values, and test_hbase_fixture_uses_pinned_upstream_inputs in tools/database/runner/tests/test_database_backends.py asserts the literal string "ARG HBASE_VERSION=2.4.18" rather than comparing the Dockerfile against HBASE_2_VERSION. So the one test that looks like it guards this coupling cannot detect the drift it appears to guard: it only proves the Dockerfile still says 2.4.18, not that the Dockerfile and _hbase.py agree. Fix: make the assertion derive from the module, asserting f"ARG HBASE_VERSION={_hbase.HBASE_2_VERSION}" and f"ARG HBASE_SHA512={_hbase._HBASE_2_SHA512}" appear in the Dockerfile, so the duplicated pin checks itself.
Analysis: HBase.__init__ always supplies HBASE_VERSION and HBASE_SHA512 as build arguments, so the Dockerfile defaults only serve a hand-run `docker build` and can disagree with the module without any runner build noticing. Deriving the two assertions from HBASE_2_VERSION and _HBASE_2_SHA512 makes the test compare the two definitions instead of repeating one of them, and it also gives the checksum a value to match rather than only requiring the ARG to exist. The literal in the HBASE_2_IMAGE assertion stays, so a version bump still has to come through this test deliberately.
Upsides: A bump to HBASE_2_VERSION or _HBASE_2_SHA512 that leaves the Dockerfile behind now fails, naming the file that was missed. The checksum default is checked for its value rather than only for its presence.
Downsides: The test reads the module-private _HBASE_2_SHA512. That is the value under test and this is the module's own unit test, but it does couple the test to a private name.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pull request dashboard statusWaiting on the author · refreshed 2026-09-17 06:53 UTC Move out of draft to request review. Status above doesn't look right?
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds Java agent conformance coverage for Apache HBase 1.7.2 and 2.4.18 clients. The scenarios exercise Get, Put, Scan, and batch operations against the stable database semantic conventions.
The database runner builds checksum-verified local HBase fixtures for both client API lines, seeds the test table, and classifies HBase spans as
db.hbase.client.HBase fixtures bind fixed loopback ports 2181, 16000, and 16020, so only one HBase conformance package can run on a host at a time.