Vanillaxi commented on PR #1139: URL: https://github.com/apache/incubator-seata-go/pull/1139#issuecomment-5151353005
> # 中文 / Chinese > 作为基础 API,代码干净、事务语义清楚(DB-owned 与 caller-owned 两条路径分明、fail-fast、`errors.Join` 不掩盖原始错误、空 batch no-op),PR notes 也诚实说明了"AT 正确性留 follow-up"。可以合并作为地基,但有几点建议先对齐,主要是命名与实际内容的落差。 > > ### 1. 命名"semantic batch / AT / MySQL"言过其实 > `batch.go` 里**没有任何 AT 或 MySQL 特定逻辑**——`executeBatch` 就是在一个 `*sql.Tx` 上循环 `tx.ExecContext(query, arg...)`。它对任意 `database/sql` driver 都成立,所谓"AT 语义"完全是调用方传入 `SeataATMySQLDriver` 时由 driver 继承来的。当前这层的净增价值,和用户自己写个 for 循环 + 一个本地事务,差别不大。 > > 建议:要么把 PR 标题/注释改成"generic single-transaction batch helper(AT 语义由底层 seata driver 提供)",要么明确说明真正的"semantic batch"(整批合并成一份 before/after image、一个 undo log)是 follow-up。否则 reviewer 会以为这里有 AT 专属实现,实际没有。 > > ### 2. `batch_at_test.go` 测的其实是 driver 行为,不是 batch.go 的 AT 性 > 测试断言"单个本地事务 + commit 一次",这验证的是 `database/sql` 的事务语义 + AT driver,而不是 `batch.go` 自己的 AT 逻辑(它没有)。建议在 `batch.go` 顶部加一行注释,说明该 API 对任意 driver 通用、AT 语义来自 seata driver,避免读者误解耦合关系。 > > ### 3. AT 分支粒度值得在文档里点明 > `ExecBatchContext` 用 `db.BeginTx` 开一个本地事务、N 条语句都在里面 → 在全局 AT 事务下这会是**一个 AT 分支、N 条 undo log**(而不是 N 个分支)。这其实是这个 API 的真正价值(减少分支数),建议在 doc comment 里明确写出来——这比"semantic batch"更能说明它解决了什么。 > > ### 4. 参数一致性校验(可选) > 没有校验每组 `arg` 的数量与 query 占位符是否一致,错配会在 `ExecContext` 才失败。作为 foundation 可接受,但如果想早失败 + 更好的报错,可在 `newBatchExecContext` 里加一个"所有 arg 组 arity 一致"的轻校验。 > > ## 小结 > 无 bug,方向 OK,作为地基可合。主要建议是**让命名/描述与"目前只是通用单事务批量执行、AT 语义靠 driver 继承"这个事实对齐**,并把"减少 AT 分支数"这个真实价值写进 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 — mainly 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 misreading the coupling. > > ### 3. AT branch granularity is worth documenting > `ExecBatchContext` opens one local transaction via `db.BeginTx` and runs N statements in it → under a global AT transaction this is **one AT branch with N undo-log rows** (not N branches). That's actually the real value of this API (fewer branches); state it in the doc comment — it explains what this solves far better than "semantic batch". > > ### 4. Argument-arity validation (optional) > There's no check that each `arg` group's count matches the query placeholders; a mismatch only fails at `ExecContext`. Acceptable for a foundation, but if you want fail-fast with a clearer error, add a light "all arg groups have consistent arity" check in `newBatchExecContext`. > > ## Summary > No bugs, direction is fine, mergeable as a base. The main ask is to **align the naming/description with the fact that this is currently a generic single-transaction batch helper with AT semantics inherited from the driver**, and to document the real value (fewer AT branches). Thanks for your review. I've fixed it. -- 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]
