terrymanu commented on PR #38682: URL: https://github.com/apache/shardingsphere/pull/38682#issuecomment-4753089948
Thanks for the explanation. I understand the motivation of this change: nativeTest repeatedly resets / closes the transaction manager, and after `ContextManager.close()`, the resource in `TransactionRule` is cleared, while the later JDBC close / cleanup path may still touch the transaction context. My concern is not that this nativeTest scenario should not be handled. The concern is that the fix currently lands in the shared production transaction abstraction, `ConnectionTransaction`. With the current implementation, when XA/BASE sees `rule.getResource() == null`, `distributedTransactionManager` becomes `null`, and then `begin()` / `commit()` / `rollback()` silently no-op. This models the lifecycle state “transaction resource has already been closed” as “a valid XA/BASE transaction that simply does not need transaction operations.” So I suggest separating the problem into two possible directions: 1. If this state only happens during nativeTest cleanup / shutdown, please try to keep the fix inside the cleanup boundary, for example by preserving the close ordering, avoiding post-close connections from re-entering transaction paths, or explicitly handling this shutdown-only case in the test utility layer. 2. If this state really needs to be accepted by production paths, then please do not model it implicitly with `null manager + no-op`. Instead, introduce an explicit closed / absent transaction resource semantics, and add tests for paths such as `setAutoCommit(false)`, `begin`, `commit`, and `rollback` to prove that behavior is intentional. In other words, I am not asking for a simple rollback. I am asking that we avoid letting a nativeTest lifecycle sentinel become shared JDBC / Proxy transaction semantics. For the ClickHouse release note, I understand that it is an optional driver and not a mandatory default dependency. The main point here is not to describe it as a major runtime change. The issue is that the current release note says `nativeTest`, while the diff changes the root managed version in `pom.xml`, and the Proxy / Proxy Native `db-clickhouse` runtime profiles consume that managed version. The user docs are also updated to `0.9.8`. So this can be handled in a lightweight way: either reword the release note to say that the optional ClickHouse JDBC driver is updated to `0.9.8`, without limiting it to nativeTest; or, if the intention is truly to affect only nativeTest, please constrain the version change to the nativeTest dependency scope. I think the first transaction-resource state issue is the one that needs to be settled first. The ClickHouse point is mainly about keeping the release note description consistent with the actual diff scope. -- 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]
