Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

init xa trx log #835

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

init xa trx log #835

wants to merge 7 commits into from

Conversation

luky116
Copy link
Contributor

@luky116 luky116 commented Mar 9, 2024

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?:


Copy link

cr-gpt bot commented Mar 9, 2024

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
Copy link

codecov-commenter commented Mar 9, 2024

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.

Copy link

cr-gpt bot commented Mar 10, 2024

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

Copy link

cr-gpt bot commented Mar 23, 2024

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

Copy link

cr-gpt bot commented Mar 23, 2024

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

Copy link

cr-gpt bot commented Mar 23, 2024

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

Copy link

cr-gpt bot commented Mar 23, 2024

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

Copy link

cr-gpt bot commented Mar 23, 2024

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

Copy link

sonarcloud bot commented Mar 23, 2024

Quality Gate Passed 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

@luky116 luky116 changed the title 【WIP】init trx log init xa trx log Mar 23, 2024
@dongzl dongzl added transaction Transaction enhancement New feature or request labels Mar 23, 2024
@dongzl dongzl added this to the 0.3.0 milestone Mar 23, 2024
_ TxState = iota
TrxStarted // CompositeTx Default state
TrxPreparing // All SQL statements are executed, and before the Commit statement executes
TrxPrepared // All SQL statements are executed, and before the Commit statement executes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"fmt"
rcontext "github.com/arana-db/arana/pkg/runtime/context"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import style

@PangXing
Copy link
Contributor

PangXing commented Jun 8, 2024

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

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 filepkg/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 linedeleteBranchSql = "DELETE FROM __arana_global_trx_log WHERE trx_id = ? and branch_id=?"

relevant filepkg/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 linefunc (gm *TxLogManager) ScanBranchTxLog(pageNo, pageSize uint64, conditions []Condition) (uint32, []BranchTrxLog, error) {

relevant filepkg/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 linefunc (gm *TxLogManager) runCleanBranchTxLogTask() {

@dongzl
Copy link
Contributor

dongzl commented Jun 8, 2024

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request transaction Transaction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

【XA】Initialization of Arana global transaction table
6 participants