github-actions[bot] commented on PR #1135:
URL:
https://github.com/apache/incubator-seata-go/pull/1135#issuecomment-5301335262
> Bot detected the issue body's language is not English, translate it
automatically.
> Note: original text was truncated due to length.
> #/Chinese
> This is the correct successor to # 1074.The line format sequence of the
kernel has been corrected - `branchType → resourceId → saveDays`, which is
exactly the same as Java `UndoLogDeleteRequestCodec', and the decoding failure
returns nil. The quality of the processor is significantly higher:
>
> * Non-AT branch skipped
> * * * `saveDays < = 0` Reject at entrance * * - this is the key safety
point, otherwise cutoff will be pushed to today/future, delete all undo logs
> * Resource not → skipped in this client cache (correct handling of TC
broadcast semantics)
> * Check for undo_log table presence
> * batch delete loop drain to `affected < batchSize`
> * `safeGetResourceManager` recover pocket
>
> I prefer approve, a few suggestions:
>> # # # 1. (Suggest pre-merger processing) `delete... limit` is MySQL
dialect, PG will report an error
> `` `go
> deleteSQL: = fmt.Sprintf ("delete from %s where log_created < =? Limit %d
", undoLogTable, batchSize)
> ```
>
> Postgres does not support `delete... limit`.PR body says PG leaves a
follow-up, but the current code sends this SQL to any AT resource - if this
client manages a PG AT resource, each TC cleanup cycle will report a SQL
error.Now that you have `res.GetDbType ()`, it is recommended to * * explicitly
gate to MySQL * *, other dbType warn + skip, instead of sending a SQL that is
doomed to fail.(If seata-go AT currently only supports MySQL, it will not
actually trigger, but the defensive gate is more stable and leaves a good
extension point for PG follow-up.)
>> # # # 2. `undoLogTable` Go `fmt.Sprintf` Stitch into SQL
> `batchSize` is int, safe; `undoLogTable` is from config, high confidence,
but it is recommended to do an identifier check or keep the hardcode default to
avoid the injection surface when the config is not contaminated by trusted
sources in the future.
>
> # # # 3. There is no upper limit/time budget for the drain cycle
> `for {}` will delete all expired lines at once.If the backlog is very
large (millions), it will take up a connection for a long time and synchronize
many rounds in the processor goroutine.It is recommended to add a `ctx`
deadline or maximum number of iterations protection to avoid delaying the
client for too long with a single TC request.
>
> # # # 4. It is recommended to add golden-bytes decoding use case
> Same as the suggestion given to # 1074: In addition to round-trip, add a
use case that uses Java to decode the hard-coded byte sequence of the copy to
truly lock the deadline format and prevent the sequence from falling back
again.You already have sqlmock's end-to-end test, which is very complete.
>> # # # 5. Close # 1074
> Since this PR "Continues # 1074/Closes # 1134" and the sequence bug of #
1074 has been fixed, it is recommended to turn off # 1074 after merging this PR
to avoid duplicate registration of codecs.
>
> # # Summary
> Direction and realization are both correct, significantly better than #
1074.Point 1 (PG dialect gate) suggests merging before processing, the rest can
be followed up.
>
> # English
> This is the correct successor to # 1074. The core wire order is now fixed
— `branchType → resourceId → saveDays`, matching Java
`UndoLogDeleteRequestCodec` exactly, and it returns nil on decode failure. The
processor is noticeably higher quality:
>
> * Skips non-AT branches> * * * Rejects` saveDays < = 0 `at the entry * *
-a key: [redacted] point; otherwise the cutoff moves to today/the future and
deletes all undo logs
> * Resource not in this client's cache → skip (correctly handles TC
broadcast semantics)
> * Checks the undo_log table exists
> * Batch-delete loops until `affected < batchSize`
> * `safeGetResourceManager` recovers defensively
>
> I'm inclined to approve. A few suggestions:
>> # # # 1. (Worth handling before merge) `delete... limit` is
MySQL-specific; PG errors
> `` `go
> deleteSQL: = fmt.Sprintf ("delete from %s where log_created < =? Limit %d
", undoLogTable, batchSize)
> ```
>> Postgres doesn't support `delete... limit`. The PR body defers PG to a
follow-up, but the current code issues this SQL for any AT resource — if this
client manages a PG AT resource, every TC cleanup cycle throws a SQL error.
Since you already have `res.GetDbType ()`, explicitly * * gate to MySQL * * and
warn + skip other dbTypes rather than emitting SQL that's guaranteed to fail.
(If seata-go AT only supports MySQL today it won't trigger, but a defensive
gate is safe
--
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]
