github-actions[bot] commented on PR #1139: URL: https://github.com/apache/incubator-seata-go/pull/1139#issuecomment-5151353908
> Bot detected the issue body's language is not English, translate it automatically. > Note: original text was truncated due to length. > #/Chinese > As the basic API, the code is clean and the transaction semantics are clear (DB-owned and caller-owned two paths are clear, fail-fast, `errors.Join` does not mask the original error, empty batch no-op), PR notes also honestly explain that "AT correctness retains follow-up".It can be merged as a foundation, but there are a few suggestions to align first, mainly the gap between the name and the actual content. > > # # # 1. Naming "semantic batch/AT/MySQL" is overstated > `batch.go` * * doesn't have any AT or MySQL specific logic * * - `executeBatch` just loops` tx.ExecContext (query, arg...) `on a' * sql.Tx `.It is established for any `database/sql` driver, and the so-called 'AT semantics' are completely inherited by the driver when the caller passes in `SeataATMySQLDriver'.The current net added value of this layer is not much different from the user writing a for loop + a local transaction. >> Recommendation: Either change the PR title/comment to "generic single-transaction batch helper (AT semantics provided by the underlying seata driver)" or explicitly state that the real "semantic batch" (batch merged into one before/after image, one undo log) is follow-up.Otherwise, the reviewer will think that there is an AT exclusive implementation here, but in fact there is none. > > # # # 2. `batch_at_test.go` is actually testing driver behavior, not the AT nature of batch.go > The test asserts "single local transaction + commit once", which verifies the transaction semantics of `database/sql` + AT driver, not 'batch.go` 's own AT logic (it doesn't).It is recommended to add a line comment at the top of `batch.go' to explain that the API is generic to any driver and the AT semantics are from the seata driver, to avoid readers misunderstanding the coupling relationship. > > # # # 3. AT branch granularity is worth highlighting in the documentation> `ExecBatchContext` opens a local transaction with `db.BeginTx', N statements are in it Under the → global AT transaction, this will be * * an AT branch, N undo logs * * (instead of N branches).This is actually the real value of this API (reducing the number of branches), it is recommended to write it clearly in the doc comment - this is more than "semantic batch" to explain what it solves. > > # # # 4. Parameter consistency check (optional) > If the number of `arg` per group is not checked to be consistent with the query placeholder, the mismatch will fail at `ExecContext'.Acceptable as a foundation, but if you want to fail early + better error reporting, you can add a light check with "all arg groups arity consistent" in `newBatchExecContext'. > > # # Summary > No bugs, direction OK, can be used as a foundation.The main recommendation is to * * align the naming/description with the fact that "currently only generic single-transaction batch execution, AT semantics are inherited by the driver" * *, and write the real value of "reducing the number of AT branches" into the doc. > > # English> As a foundation API the code is clean with clear transaction semantics (DB-owned vs caller-owned paths cleanly separated, fail-fast, `errors.Join` doesn't hide the original error, empty batch is a no-op), and the PR notes honestly defer "AT correctness" to a follow-up. Mergeable as a base, but a few things to align first — mostly the gap between the naming and the actual content. > > # # # 1. The "semantic batch/AT/MySQL" naming oversells it> `batch.go` has * * no AT- or MySQL-specific logic * * — `executeBatch` just loops` tx.ExecContext (query, arg...) `on a single` * sql.Tx `. It works for any `database/sql` driver; the "AT semantics" are entirely inherited from the caller passing `SeataATMySQLDriver`. The net new value of this layer over "a user writing a for-loop inside one local transaction" is currently thin. >> Suggestion: either retitle to "generic single-transaction batch helper (AT semantics provided by the underlying seata driver)", or make explicit that the real "semantic batch" (one merged before/after image and undo log for the whole batch) is the follow-up. Otherwise reviewers assume there's AT-specific implementation here when there isn't. > > # # # 2. `batch_at_test.go` tests driver behavior, not batch.go's AT-ness> The test asserts "single local transaction + commit once", which verifies` database/sql 'tx semantics + the AT driver, not any AT logic in `batch.go` (there is none). Add a comment at the top of `batch.go` stating the API is driver-agnostic and AT semantics come from the seata driver, to avoid misr -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
