init xa trx log

[luky116]

What this PR does:

Build an overall code framework for global transactions

Which issue(s) this PR fixes:

Fixes #829

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 20.98765% with 128 lines in your changes are missing coverage. Please review.

Project coverage is 40.12%. Comparing base (a27c47b) to head (5a62088).

Files	Patch %	Lines
pkg/runtime/transaction/trx_log.go	29.41%	48 Missing ⚠️
pkg/runtime/tx.go	0.00%	37 Missing ⚠️
pkg/runtime/transaction/hook.go	0.00%	29 Missing ⚠️
pkg/runtime/context/context.go	66.66%	7 Missing ⚠️
pkg/runtime/transaction/fault_decision.go	0.00%	4 Missing ⚠️
pkg/runtime/transaction/xa.go	0.00%	3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #835      +/-   ##
==========================================
- Coverage   40.20%   40.12%   -0.08%     
==========================================
  Files         265      265              
  Lines       26708    26908     +200     
==========================================
+ Hits        10738    10797      +59     
- Misses      14823    14964     +141     
  Partials     1147     1147              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

[sonarcloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
7.1% Duplication on New Code

See analysis details on SonarCloud

[baerwang]

这两个comment note是不是要改一下

[baerwang]

import style

[PangXing]

@CodiumAI-Agent /review

[CodiumAI-Agent]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, due to the complexity and breadth of the changes involving transaction management, database schema updates, and the introduction of new transaction logs. The PR modifies critical components which require careful review to ensure system stability and data integrity.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The deleteBranchSql uses the wrong table name in its SQL statement. It should target the __arana_branch_trx_log table instead of __arana_global_trx_log.
	Inconsistency: The ScanBranchTxLog method and runCleanBranchTxLogTask are not implemented but are planned in the TODO comments. This could lead to incomplete functionality if not addressed.
🔒 Security concerns	No

Code feedback:

---

relevant file	pkg/runtime/transaction/trx_log.go
suggestion	Correct the SQL table name in the deleteBranchSql statement to point to the correct table for branch transaction logs. This is important to ensure that the deletion operation affects the intended data. [important]
relevant line	deleteBranchSql = "DELETE FROM __arana_global_trx_log WHERE trx_id = ? and branch_id=?"

---

relevant file	pkg/runtime/transaction/trx_log.go
suggestion	Implement the ScanBranchTxLog method to ensure that branch transaction logs can be queried effectively. This is important for maintaining the functionality and integrity of transaction log management. [important]
relevant line	func (gm *TxLogManager) ScanBranchTxLog(pageNo, pageSize uint64, conditions []Condition) (uint32, []BranchTrxLog, error) {

---

relevant file	pkg/runtime/transaction/trx_log.go
suggestion	Implement the runCleanBranchTxLogTask method to ensure that branch transaction logs are cleaned up appropriately. This is crucial for preventing data buildup and ensuring system performance. [important]
relevant line	func (gm *TxLogManager) runCleanBranchTxLogTask() {

---

[dongzl]

@CodiumAI-Agent /improve

[CodiumAI-Agent]

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Set transaction state to 'TrxAborted' in error handling to ensure state consistency Ensure that the transaction state is set to rcontext.TrxAborted in the error handling of doPrepareCommit to maintain consistency with the transaction state management. pkg/runtime/tx.go [320-340] func (tx *compositeTx) doPrepareCommit(ctx context.Context) error { var g errgroup.Group for k, v := range tx.txs { g.Go(func() error { return v.Prepare(ctx) }) } if err := g.Wait(); err != nil { + tx.setTxState(ctx, rcontext.TrxAborted) return err } tx.setTxState(ctx, rcontext.TrxPrepared) return nil } Suggestion importance[1-10]: 10 Why: Setting the transaction state to rcontext.TrxAborted in the error handling ensures consistency in transaction state management, which is critical for maintaining the integrity of the transaction process.	10
	Improve type safety in transaction status retrieval The getTxStatus function casts the context value to int32 before comparing it with transaction states, which are of type TxState. To maintain type safety and avoid potential runtime errors, directly compare the context value with the TxState type. pkg/runtime/context/context.go [176-178] -if data, ok := ctx.Value(v).(int32); ok { - if data >= int32(TrxStarted) && data <= int32(TrxAborted) { - return TxState(data) +if data, ok := ctx.Value(v).(TxState); ok { + if data >= TrxStarted && data <= TrxAborted { + return data } } Suggestion importance[1-10]: 9 Why: This suggestion improves type safety and reduces the risk of runtime errors by ensuring that the context value is directly compared with TxState type. This is a significant enhancement for code robustness.	9
	Enhance error handling in the context string retrieval function The getString function should handle the case where the context value is not a string, by either logging an error or returning a default value indicating an error. This would improve error handling and debugging capabilities. pkg/runtime/context/context.go [169-172] if data, ok := ctx.Value(v).(string); ok { return data } -return "" +log.Printf("Expected string in context for key %v, found none", v) +return "ERROR: Invalid type" Suggestion importance[1-10]: 7 Why: The suggestion enhances error handling and debugging capabilities by logging an error when the context value is not a string. While useful, it is a minor improvement compared to other suggestions.	7
	Implement the function to handle branch transaction logs instead of using a panic Replace the panic placeholder in AddOrUpdateBranchTxLog with a proper implementation to handle branch transaction logs. pkg/runtime/transaction/trx_log.go [137-139] func (gm *TxLogManager) AddOrUpdateBranchTxLog(l BranchTrxLog) error { - panic("implement me") + // Example implementation + // Convert BranchTrxLog fields to proto.Values and call the database + // This is a placeholder and should be replaced with actual logic + return nil } Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies the need to replace the panic placeholder with a proper implementation. However, the provided implementation is still a placeholder and does not offer a complete solution.	7
	Implement the cleanup task for branch transaction logs Implement the runCleanBranchTxLogTask function to handle the cleanup of branch transaction logs. pkg/runtime/transaction/trx_log.go [257-258] func (gm *TxLogManager) runCleanBranchTxLogTask() { - panic("implement me") + // Example implementation + // Logic to clean up old or completed branch transaction logs + // This is a placeholder and should be replaced with actual logic } Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies the need to implement the cleanup task for branch transaction logs. However, the provided implementation is still a placeholder and does not offer a complete solution.	7
Possible bug	Prevent potential nil pointer dereference in transaction log handling The onStarted function sets the transaction ID and other properties on trxLog but does not check if trxLog is initialized. This can lead to a nil pointer dereference. Ensure trxLog is initialized before setting properties on it. pkg/runtime/transaction/hook.go [98-102] +if xh.trxLog == nil { + xh.trxLog = &GlobalTrxLog{} +} xh.trxLog.TrxID = tx.GetTrxID() xh.trxLog.Status = tx.GetTxState() xh.trxLog.Tenant = tx.GetTenant() xh.trxLog.StartTime = tx.GetStartTime() xh.trxLog.ExpectedEndTime = tx.GetExpectedEndTime() Suggestion importance[1-10]: 10 Why: This suggestion addresses a potential bug that could lead to a nil pointer dereference, which is a critical issue. Ensuring trxLog is initialized before setting properties is essential for preventing runtime errors.	10
	Add checks for nil context and BackendConnection to prevent runtime errors Consider checking for a nil context or a nil BackendConnection before proceeding with the transaction operations to avoid potential runtime panics. pkg/runtime/transaction/xa.go [36-44] func StartXA(ctx context.Context, bc *mysql.BackendConnection) (proto.Result, error) { + if ctx == nil || bc == nil { + return nil, errors.New("invalid input parameters") + } txId := rcontext.TransactionID(ctx) if len(txId) == 0 { return nil, ErrorInvalidTxId } if rcontext.TransactionStatus(ctx) != rcontext.TrxStarted { return nil, ErrorInvalidTxStatus } return bc.ExecuteWithWarningCount(fmt.Sprintf("XA START '%s'", txId), false) } Suggestion importance[1-10]: 9 Why: Adding checks for nil context and BackendConnection is crucial to prevent potential runtime panics, which enhances the robustness of the function.	9
Bug fix	Correct the SQL statement to properly reference the branch transaction log table and columns Correct the SQL statement in DeleteBranchTxLog to reference the correct table and use the correct column names for branch transaction logs. pkg/runtime/transaction/trx_log.go [84] -deleteBranchSql = "DELETE FROM __arana_global_trx_log WHERE trx_id = ? and branch_id=?" +deleteBranchSql = "DELETE FROM __arana_branch_trx_log WHERE txr_id = ? and branch_id=?" Suggestion importance[1-10]: 10 Why: This suggestion addresses a critical bug by correcting the SQL statement to reference the correct table and column names, ensuring proper functionality.	10
Possible issue	Adjust comments for transaction states to reflect distinct meanings The TrxPreparing and TrxPrepared states have identical comments which might be confusing or incorrect. Ensure that the comments accurately reflect the distinct states or adjust the state definitions if they are redundant. pkg/runtime/context/context.go [41-42] -TrxPreparing // All SQL statements are executed, and before the Commit statement executes -TrxPrepared // All SQL statements are executed, and before the Commit statement executes +TrxPreparing // All SQL statements are executed, preparing for commit +TrxPrepared // All SQL statements are executed, ready for commit Suggestion importance[1-10]: 8 Why: The suggestion correctly identifies that the comments for TrxPreparing and TrxPrepared are identical and potentially confusing. Adjusting the comments to reflect distinct meanings improves code readability and maintainability.	8
Documentation	Add meaningful documentation to the TxLogManager struct Replace the placeholder TODO comment in TxLogManager struct with actual documentation describing the struct. pkg/runtime/transaction/trx_log.go [89-92] -// TODO +// TxLogManager manages transaction logs for both global and branch transactions. type TxLogManager struct { sysDB proto.DB } Suggestion importance[1-10]: 8 Why: Adding meaningful documentation improves code readability and maintainability, making it easier for other developers to understand the purpose of the TxLogManager struct.	8
Maintainability	Use a configuration parameter for transaction timeout to improve flexibility Replace the hardcoded transaction timeout with a configurable value to enhance flexibility and maintainability. pkg/runtime/tx.go [121-138] -func newCompositeTx(ctx context.Context, pi *defaultRuntime, hooks ...TxHook) *compositeTx { +func newCompositeTx(ctx context.Context, pi *defaultRuntime, hooks ...TxHook, config *Config) *compositeTx { now := time.Now() tx := &compositeTx{ tenant: rcontext.Tenant(ctx), id: gtid.NewID(), rt: pi, txs: make(map[string]*branchTx), beginTime: now, - expectedEndTime: now.Add(time.Second * 30), + expectedEndTime: now.Add(config.GlobalTransactionTimeout), hooks: hooks, beginFunc: func(ctx context.Context, bc *mysql.BackendConnection) (proto.Result, error) { return bc.ExecuteWithWarningCount("begin", true) }, } tx.setTxState(ctx, rcontext.TrxStarted) return tx } Suggestion importance[1-10]: 8 Why: Using a configuration parameter for the transaction timeout improves the flexibility and maintainability of the code, making it easier to adjust the timeout without modifying the source code.	8
Best practice	Implement error handling for transaction state change hooks to manage hook failures Add error handling for the OnTxStateChange method within the setTxState function to handle potential errors from transaction state change hooks. pkg/runtime/tx.go [445-448] -func (tx *compositeTx) setTxState(ctx context.Context, state rcontext.TxState) { +func (tx *compositeTx) setTxState(ctx context.Context, state rcontext.TxState) error { tx.txState = state for i := range tx.hooks { - tx.hooks[i].OnTxStateChange(ctx, state, tx) + if err := tx.hooks[i].OnTxStateChange(ctx, state, tx); err != nil { + return err + } } + return nil } Suggestion importance[1-10]: 7 Why: Implementing error handling for transaction state change hooks is a good practice to manage potential hook failures, ensuring that errors are properly propagated and handled.	7