RaigorJiang commented on PR #38680:
URL: https://github.com/apache/shardingsphere/pull/38680#issuecomment-4411918494
• Decision
Merge Verdict: Mergeable
Reviewed Scope:
- PR latest head: 17614b1391aac98e94f93f1d897dd455e9a86b7c
- Compared range: apache/master...HEAD
- Production code:
-
proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/context/BackendExecutorContext.java:45
- Test code:
-
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/context/BackendExecutorContextTest.java:48
-
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutorTest.java:187
-
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/DatabaseProxyConnectorFactoryTest.java:67
-
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnectorTest.java:159
-
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/handler/distsql/rul/PreviewExecutorTest.java:109
-
proxy/bootstrap/src/test/java/org/apache/shardingsphere/proxy/initializer/BootstrapInitializerTest.java:133
-
proxy/frontend/dialect/postgresql/src/test/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/PostgreSQLBatchedStatementsExecutorTest.java:132
-
proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/text/query/MySQLMultiStatementsProxyBackendHandlerTest.java:106
- Runtime topology reviewed: ShardingSphere-Proxy, Standalone and Cluster
lifecycle, shared backend executor used by
MySQL/PostgreSQL/openGauss/Firebird/generic backend paths.
- CI metadata reviewed from GitHub PR #38680 latest head.
Not Reviewed Scope:
- No manual native-image runtime execution beyond CI signal.
- No manual real-client repeated Proxy process smoke beyond the PR tests
and CI E2E matrix.
- No SQL parser semantic documentation review was needed because this PR
does not change parser grammar, visitor behavior, SQL syntax acceptance, name
resolution, routing precedence, or
default schema semantics.
Need Expert Review:
- No dedicated security, SQL dialect, or parser expert review required.
- Concurrency/runtime reviewer is optional but not blocking; the change is
a small synchronized lifecycle-state adjustment around an existing singleton.
Basis
Root-cause chain is now addressed directly, not only by fallback behavior.
Original failure model:
- ShardingSphereProxy.close() calls BackendExecutorContext.shutdown() from
proxy/frontend/core/src/main/java/org/apache/shardingsphere/proxy/frontend/ShardingSphereProxy.java:165.
- Commit 7a75e3931f6d404af7ee299c0112d997a0e22230 made shutdown close the
executor, but request-side paths could still interact ambiguously with the
singleton lifecycle on repeated Proxy
startup in the same JVM.
- Request-side consumers include:
- ProxySQLExecutor, used by standard backend connector execution.
- PostgreSQLBatchedStatementsExecutor, PostgreSQL extended query batch
path.
- MySQLMultiStatementsProxyBackendHandler, MySQL text multi-statement
path.
- PreviewExecutor, DistSQL preview path.
- The unsafe behavior to prevent is using, recreating, or implicitly
reviving a backend executor from a request-side lookup after shutdown, instead
of requiring explicit Proxy lifecycle
initialization.
The PR now makes the lifecycle boundary explicit:
- init() closes any previous executor, creates a fresh one, and moves
state to RUNNING.
- shutdown() closes and clears the current executor, then moves state to
CLOSED.
- getExecutorEngine() rejects lookup while CLOSED.
- CLOSED -> RUNNING is allowed only through explicit init() from bootstrap
lifecycle, not request-side executor lookup.
This is design-consistent with
proxy/bootstrap/src/main/java/org/apache/shardingsphere/proxy/initializer/BootstrapInitializer.java:55,
where backend executor initialization happens after
ProxyContext.init(contextManager) and before frontend serving starts.
Risk scan result:
- Design risk: acceptable. The public ambiguous close() method is removed
from BackendExecutorContext, leaving one explicit lifecycle shutdown boundary.
Existing external production
references to close() were not found.
- Performance risk: low. The only new production behavior is an extra
synchronized closeExecutorEngine() call in init(), which runs at bootstrap
lifecycle time, not SQL hot path.
getExecutorEngine() keeps the same synchronized lookup shape.
- Compatibility risk: low. No config key, SPI contract, SQL grammar,
routing order, default schema, or dialect semantics changed.
kernel-executor-size is still read from the same
ConfigurationPropertyKey.KERNEL_EXECUTOR_SIZE.
- Functional regression risk: acceptable. Request-side lookup after
shutdown now fails fast, which is the intended closed-state behavior. Explicit
repeated bootstrap restores the executor.
- Cross-dialect/shared-path risk: reviewed as shared Proxy executor blast
radius. Generic backend connector, PostgreSQL extended batch, MySQL
multi-statement, and DistSQL preview paths are
covered. openGauss/Firebird/protocol-adjacent frontend paths still share
the same executor lifecycle but do not receive dialect-specific behavior
changes; their risk is bounded by the
generic BackendExecutorContext and ProxySQLExecutor coverage.
- Feature-flag/config-disabled path: no new flag or disabled-path branch
is introduced. Existing kernel-executor-size=0 / configured-size behavior still
flows through
ExecutorEngine.createExecutorEngineWithSize(...).
Adversarial pass:
- Cross-dialect or adjacent-feature regression path: checked MySQL
adjacent path and PostgreSQL target path; CI also includes PostgreSQL/MySQL
Proxy E2E smoke and generic backend-core
tests.
- Config-disabled or feature-flag-off path: no new flag; default
kernel-executor-size remains unchanged and is still obtained from existing
metadata props.
- Original symptom partially covered concern: closed. The PR now has
direct RUNNING -> CLOSED -> RUNNING state-machine coverage, bootstrap repeated
lifecycle coverage, and request-side
execution after repeated lifecycle through PostgreSQL and MySQL paths.
No unrelated PR file changes were found in the reviewed range. The only
uncommitted local file is PR-38665-REVIEW-FOLLOWUP.md, which is not part of PR
#38680 latest head and was excluded
from the review.
Pre-Merge Checks
GitHub PR status reviewed:
- Latest commits:
- c99d8d748e5386d44fbb6fe1db1fd0f43e37a6ed
- 17614b1391aac98e94f93f1d897dd455e9a86b7c
- GitHub checks on latest head:
- Required CheckStyle: success
- Required Spotless: success
- License: success
- CI: success
- E2E SQL smoke for Proxy Standalone/Cluster, MySQL/PostgreSQL: success
- E2E Operation and Agent matrix: success
- GitHub mergeStateStatus reports BLOCKED, but the visible check rollup is
green; this appears to be repository process/merge-policy state rather than a
code or CI failure.
Local verification on the same code head:
./mvnw -pl proxy/backend/core -am -DskipITs -Dspotless.skip=true
-Dtest=BackendExecutorContextTest,DatabaseProxyConnectorFactoryTest,StandardDatabaseProxyConnectorTest,PreviewExecutorTest
-Dsurefire.failIfNoSpecifiedTests=false test
Result: exit code 0, BUILD SUCCESS, 49 tests passed.
./mvnw clean install -B -Dremoteresources.skip
Result: exit code 0, BUILD SUCCESS, total time 08:22 min.
./mvnw checkstyle:check -Pcheck -T1C
Result: exit code 0, BUILD SUCCESS.
./mvnw spotless:check -Pcheck -T1C
Result: exit code 0, BUILD SUCCESS.
Multi-round comparison:
- Previous concern, direct RUNNING -> CLOSED -> RUNNING proof: fixed by
BackendExecutorContextTest.assertInitAfterShutdown.
- Previous concern, request-side lookup should not revive executor after
shutdown: fixed by getExecutorEngine() rejecting CLOSED, with tests asserting
IllegalStateException.
- Previous concern, PostgreSQL near-production lifecycle path: fixed by
PostgreSQLBatchedStatementsExecutorTest.assertExecuteBatchAfterRepeatedProxyLifecycle.
- Previous concern, MySQL adjacent shared path: fixed by
MySQLMultiStatementsProxyBackendHandlerTest.assertExecuteAfterRepeatedProxyLifecycle.
- Previous CI failure from singleton CLOSED leakage into connector tests:
fixed by explicit test setup/cleanup in DatabaseProxyConnectorFactoryTest,
StandardDatabaseProxyConnectorTest, and
PreviewExecutorTest.
- Newly introduced issue: none found in the reviewed scope.
Self-check: review used the PR latest version only, covered root cause
before fallback behavior, included shared-path blast-radius review, checked an
adjacent non-target path, verified CI/
local evidence with -am where scoped, ignored only the project-specific
git diff --check whitespace noise as instructed, and found no current
merge-blocking code issue.
--
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]
