lymerin commented on PR #7513:
URL: https://github.com/apache/shenyu/pull/7513#issuecomment-6099321309
> **Re-review of the rebase `c18f8bbae` → `bd2f9e4b`.**
>
> * The production patch (`AbstractLogPluginDataHandler.java`) and
`LoggingElasticSearchPluginDataHandlerTest.java` are byte-identical to what was
reviewed before — no behavioral change.
> * The changed test (`AbstractLogPluginDataHandlerTest.java`) removes the
reflection into `Singleton.SINGLES` per the earlier note and instead seeds a
stale `TestLogConfig` via `Singleton.INST.single(...)` in `@BeforeEach`. I
traced the assertions: the first enable takes the refresh path via the
`!PLUGIN_GLOBAL_CONFIG_MAP.containsKey(id)` clause (config also differs, "0.5"
vs "1"), so `initializationCount == 2` after re-enable still holds; repeated
enable does not re-initialize; resume is proven end-to-end via
`collector.collect` → `consumed.await`. The seed actually strengthens the test
— re-enable now exercises the `containsKey` clause in isolation with an
already-equal config.
> * Dropping the 4 test files in modules migrated to the plugin store is a
rebase artifact, adequately disclosed in the PR body.
>
> **Remaining open (non-blocking, carried over):**
>
> * The short explanatory comment on the `containsKey` clause ("the map
entry is the active marker; it is cleared on disable even if close() fails") is
still worth adding.
> * The check-then-refresh race in `handlerPlugin` remains (pre-existing,
previously acknowledged as not required).
>
> **New nits (non-blocking):**
>
> * `assertStarted` still reflects on private `started` / `isStarted` fields
by name — given this rebase's purpose was removing a reflection hack, a
package-visible accessor or behavioral assertion would be less brittle.
> * `@BeforeEach` seeds `Singleton.INST` but `tearDown` only clears
`getPluginGlobalConfigMap()`; the seeded Singleton entry is no longer cleaned
up (impact is low since `TestLogConfig` is file-private, but the asymmetry is a
trap for the next test added to this class).
> * The enable → disable → re-enable choreography is duplicated inline
across both tests; a small `cycle(...)` helper would DRY it up.
Thanks for the detailed review. I went through each point and made a small
follow-up without widening the lifecycle change.
1. About the `containsKey` condition
I added a comment to make the state distinction explicit.
`Singleton` is the retained configuration store and intentionally still
contains the previous config after disable, while `PLUGIN_GLOBAL_CONFIG_MAP` is
used as the active-lifecycle marker and is removed on disable, including when
`close()` fails.
So the third condition is not redundant even when:
`globalLogConfig.equals(exist) == true`
because the config can still be retained while the collector/client are no
longer active.
2. About the reflection in `assertStarted`
I agreed with this point and removed it.
Reading private `started` / `isStarted` fields made the test depend on the
current internal implementation. A future rename or replacing the
`AtomicBoolean` state with another lifecycle representation could break the
test even if behavior stayed correct.
The test now observes behavior instead:
- client initialization count
- client close count
- collector start count
- collector close count
- final real `collector.collect(...) -> consume0(...)` behavior
So the regression test now verifies:
enable
→ client initialized / collector started
disable
→ collector and client closed
re-enable with the same retained config
→ client initialized again / collector started again
repeated enable while already active
→ no extra initialization
finally
→ a log is actually consumed again
This keeps the test focused on lifecycle behavior rather than private fields.
3. About cleaning the `Singleton` state
I also agreed that leaving the seeded `TestLogConfig` in `Singleton` after
the test was asymmetric and could affect future tests.
I considered two approaches:
A. wrap `Singleton` access behind overridable methods in
`AbstractLogPluginDataHandler`, so the test could use a local config field;
B. keep the test on the real production `Singleton` path and add
`Singleton.remove(Class<?>)` for cleanup.
I chose B deliberately.
This regression specifically depends on the difference between:
- the config still being retained in the real `Singleton`
- the active marker being removed from `PLUGIN_GLOBAL_CONFIG_MAP`
Using a test-local config field would improve isolation, but it would also
bypass part of the real production path involved in the bug.
So the test now keeps the real `Singleton` behavior during:
enable -> disable -> re-enable
and only removes `TestLogConfig` in `@AfterEach`.
`Singleton.remove(Class<?>)` is also covered by its own unit test.
4. About extracting a `cycle(...)` helper
I intentionally did not extract this.
There is some duplication, but the enable -> disable -> re-enable transition
sequence is itself the behavior being documented by these regression tests.
The two tests also differ at the important middle step:
- the normal case exercises the real collector/client close path
- the failure case replaces the collector with one whose `close()` throws
and verifies the `finally` recovery path
A generic `cycle(...)` helper would either hide the important lifecycle
order or need extra parameters/callbacks for the exceptional case.
For these two tests, I think the small duplication is easier to read than
introducing another abstraction.
5. About the concurrent `handlerPlugin` race
I left this unchanged.
I agree that the check-then-refresh sequence is not atomic, but this
behavior predates this fix and is independent of the disable/re-enable
regression.
Adding synchronization here would change lifecycle/concurrency semantics and
should have separate coverage rather than being folded into this PR.
6. About the current CI failures
There are currently two CI failures that I cannot resolve with a clear
branch-side code change.
First, `it-k8s-gateway-api` failed before the actual test phase, in `Build
k8s Cluster`:
`ERROR: image: "shenyu-examples-http:latest" not present locally`
The Maven build, integrated-test build, and example build steps all
succeeded before that, and the test steps were skipped because cluster creation
failed.
There is also another `it-k8s-gateway-api` run for the same SHA (`8d0fa913`)
that succeeded, so this one looks intermittent and environment-related.
Second, the RocketMQ logging E2E also failed:
`shenyu-e2e-case-logging-rocketmq`
with:
`Failed to consume RocketMQ access log`
The failure occurs after waiting about 30 seconds for the expected access
log. The RocketMQ Name Server and broker both report successful startup in the
job log, so this is not an obvious container-startup failure.
Since this PR does touch the shared logging lifecycle, I do not want to
simply label this failure as unrelated. However, the module-level logging tests
were green locally, and this E2E has also shown intermittent behavior during
previous runs/reviews.
At the moment I do not have a deterministic reproduction or a branch-side
change that clearly fixes either CI failure without changing unrelated code.
The current SHA has already passed:
- `ci`
- `CodeQL`
- `native-sql-matrix`
- `it`
- `it-k8s`
- `k8s-examples-http`
So for the remaining failed jobs, especially the RocketMQ E2E, I think the
safest next step is to re-run them first and see whether the failure reproduces
consistently.
Could you please help re-run the failed CI jobs if appropriate?
--
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]
