BobSong-dev commented on PR #6927:
URL: https://github.com/apache/shenyu/pull/6927#issuecomment-5311323018
> LGTM.
>
> Core fix is correct: `RocketMQLogCollectClient.initClient0()` previously
registered its own `Runtime.getRuntime().addShutdownHook(new
Thread(this::close))` on every successful init, while the parent
`AbstractLogConsumeClient` already registers and centrally manages the shutdown
hook (registered at line 78, removed in `close()` at line 84). On repeated
config refreshes the child's hook would accumulate and the producer's close
logic could run multiple times at JVM exit. Removing the duplicate is the right
call, and cleanup semantics are preserved by the parent.
>
> e2e script change is a sensible hardening: staged bring-up (mysql/admin →
healthcheck → bootstrap → healthcheck → examples) with `|| exit 1` on the
healthchecks so the script fails fast on unhealthy containers instead of
silently continuing.
>
> Minor note (non-blocking): the script assumes the service names
`shenyu-mysql`, `shenyu-admin`, `shenyu-bootstrap` exist in
`shenyu-sync-${sync}-eureka.yml` and that `k8s/script/healthcheck.sh` is
present — worth a quick CI run to confirm, but it's test-only infra.
>
> Approving.
Thanks for the careful review and approval.
I also checked the point about the compose service names and the healthcheck
script with the CI run. The Spring Cloud E2E job passed with the staged startup
flow, and the full E2E workflow is green, so the referenced services and
k8s/script/healthcheck.sh are available in this scenario.
Appreciate the review.
--
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]
