anmolnar commented on PR #8809:
URL: https://github.com/apache/hbase/pull/8809#issuecomment-6635039272
> _Co-authored by Claude (Anthropic) on behalf of @anmolnar_
Took a closer look after my earlier approval. The overall approach is solid:
running each cluster in its own container is a reasonable way around the static
`META_TABLE_NAME` until #7730 lands, and the failure report with per-test HBase
logs is really useful. There are a few things I'd like addressed before merge,
plus some that could be follow-ups.
### Before merge
1. **Tests modify tracked files and strip their license headers.**
`set_hbase_conf_property_value` rewrites `cluster{1,2}/conf/hbase-site.xml` in
the checkout. `ElementTree.parse()` drops XML comments by default, so the first
run removes the ASF header from both files and leaves the tree dirty. A failed
run also leaves the read-only flags flipped for the next run. Could we copy the
configs into a scratch dir (e.g. under `tmp-read-replica-data`) and mount those
instead?
2. **Fixed container and compose project names.** `hbase-docker` /
`hbase-docker-2` and the default compose project are global on the Docker host.
`disableConcurrentBuilds()` only applies within a single job, so the master and
branch-3 jobs, or any other job on the same agent, can collide. If the 2h
timeout kills the outer `docker run`, the inner `trap cleanup` never runs, and
`cleanup_host` only removes the image. The containers then leak and the next
build fails on a name conflict. Suggest deriving `COMPOSE_PROJECT_NAME` and the
container names from `BUILD_TAG`, and having the outer cleanup also run `docker
compose down` or remove containers by label.
3. **Duplicate JIRA comments.** The post block reuses the main nightly's
`jiraComment` logic with the same "Results for branch master … +1/-1 overall"
header. Every JIRA committed to master would get a second comment that's
indistinguishable from the real nightly's. Can we drop the JIRA commenting, or
at least label it clearly as Read-Replica checks?
### Can be follow-ups
- **`read-replica/Dockerfile` builds HBase twice.** Stage 0 runs `mvn clean
install` on the full source, and stage 1 runs `mvn clean package …
assembly:single` on the same source. The cache stage gets invalidated by `COPY
./hbase` anyway. Also, because `docker build` runs on the host daemon, the
`-m/--m2` mount never reaches it, so every run re-downloads all dependencies. A
single build stage with a BuildKit `--mount=type=cache,target=/root/.m2` would
roughly halve build time.
- **`-j/--java-version` has no effect on what's tested.** It only sets
`JAVA_HOME` for the `mvn clean` step; the build and runtime always use
`ubi8/openjdk-17`. Either wire it through as a build arg or remove it.
- **Sleeps and races.** `major_compact_and_wait` can see `NONE` before the
compaction starts. The split assertions rely on `time.sleep(5)`, and the
dual-active test sleeps a fixed 60s per iteration. Combined with the
class-level `@pytest.mark.flaky(reruns=2)`, a real race in the feature can pass
on the third attempt and show green. I'd prefer polling for the expected state
and narrowing the reruns.
- **Shared CI image.** Adding `docker.io` / `docker-compose-v2` to
`dev-support/docker/Dockerfile` puts them in every nightly image. The
`docker.io='20.10.*'` pin also selects jammy's original 20.10.12 rather than
the patched version in jammy-updates. Might be worth a separate stage or image.
- **Docker socket mount.** Mounting `/var/run/docker.sock` on ASF agents is
probably worth a quick check with Infra.
- **Smaller robustness items:**
- The exec timeout abandons the thread, but `hbase shell` keeps running in
the container.
- `hbase shell -n <<< "{cmd}"` breaks on `"`, `$` or backticks.
- `list_tables` uses `ast.literal_eval` on the first `[...]` in the
output, which breaks on bracketed log lines.
- `bulkload.sh` lacks `set -e`, so a failed `ImportTsv` still runs
`completebulkload`.
- The `mvn clean` in `build-images.sh` is redundant after `rsync --exclude
target`.
- **Nits:**
- It's called "nightly" but uses the every-3-days cron.
- The `getCronParams` cases for branch-2 and other branches are
unreachable given the `when` clause.
- The client mixes `logging.info` and `logger.info`.
- The docstring path in `test_dual_active_cluster_startup.py` points to
`scripts/` instead of `test/`.
- `num_regions/2` passes a float.
- Help text typo: "by the refreshing meta".
Happy to re-approve once 1–3 are sorted. The rest could go into a follow-up
JIRA if you'd rather not hold this up.
--
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]
