jiangML commented on PR #39211: URL: https://github.com/apache/shardingsphere/pull/39211#issuecomment-5044025018
### Summary **Review Result: Mergeable** Reason: The code-scope review confirms that the fix addresses the root cause: the common advisor is loaded and can execute before the first `ContextManager` exists. The latest commit also corrects the test expectation and restores global-state isolation. This result is limited to code correctness; CI not reviewed by request. ### Evidence - [`AdvisorConfigurationLoader`](https://github.com/apache/shardingsphere/blob/0c01d6a3942c16ad7b71ed9f1557b5a54b11d80e/agent/core/src/main/java/org/apache/shardingsphere/agent/core/advisor/config/AdvisorConfigurationLoader.java#L51-L68) loads and merges `common-advisors.yaml` in addition to configured plugin advisor resources. - [`PluginContext.isPluginEnabled()`](https://github.com/apache/shardingsphere/blob/0c01d6a3942c16ad7b71ed9f1557b5a54b11d80e/agent/plugins/core/src/main/java/org/apache/shardingsphere/agent/plugin/core/context/PluginContext.java#L57-L73) returns `true` before a `ContextManager` is available, so the advisor executor does not suppress the bootstrap advice. - [`ShardingSphereDataSourceAdvice`](https://github.com/apache/shardingsphere/blob/0c01d6a3942c16ad7b71ed9f1557b5a54b11d80e/agent/plugins/core/src/main/java/org/apache/shardingsphere/agent/plugin/core/advice/common/ShardingSphereDataSourceAdvice.java#L31-L46) stores the driver context after `createContextManager` and removes it before datasource closure. - [`PluginContextTest`](https://github.com/apache/shardingsphere/blob/0c01d6a3942c16ad7b71ed9f1557b5a54b11d80e/agent/plugins/core/src/test/java/org/apache/shardingsphere/agent/plugin/core/context/PluginContextTest.java#L43-L90) covers the empty-holder, enabled-driver, disabled-driver, and disabled-proxy scenarios, while restoring holder, `PluginContext`, and `ProxyContext` state after each test. - Focused verification passed: `PluginContextTest` ran 4 tests successfully; module Spotless and Checkstyle checks also passed. ### Review Details - Review Focus: Code Correctness Review. CI not reviewed by request. - Reviewed Scope: All 10 changed files at head `0c01d6a3942c16ad7b71ed9f1557b5a54b11d80e`; local merge-base `9bd9eaa6e55d96e5db03d9fdac4d359795ba8829`; the local triple-dot file list matched GitHub `/pulls/39211/files`. - Not Reviewed Scope: Driver-process runtime validation and GitHub Actions were not reviewed. - Verification: PR ref refresh, public PR metadata/file/discussion retrieval, latest-diff source tracing, focused test execution, Spotless, and Checkstyle completed successfully. - Release Note / User Docs: Not required; this change introduces no user-facing configuration, API, or migration requirement. -- 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]
