-
Notifications
You must be signed in to change notification settings - Fork 736
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
Introduce transactionAutoCommitTimeout
and transactionCleanupTimeout
configuration values for transactions
#42940
base: master
Are you sure you want to change the base?
Introduce transactionAutoCommitTimeout
and transactionCleanupTimeout
configuration values for transactions
#42940
Conversation
This commit introduces two new configurations values for transactions, `transactionAutoCommitTimeout` and `transactionCleanupTimeout`
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42940 +/- ##
============================================
- Coverage 77.29% 77.29% -0.01%
- Complexity 51350 51357 +7
============================================
Files 2932 2932
Lines 204512 204545 +33
Branches 26692 26716 +24
============================================
+ Hits 158084 158103 +19
- Misses 37838 37846 +8
- Partials 8590 8596 +6 ☔ View full report in Codecov by Sentry. |
transactionAutoCommitTimeout
and transactionCleanupTimeout
configuration values for transactionstransactionAutoCommitTimeout
and transactionCleanupTimeout
configuration values for transactions
...rina-runtime/src/main/java/io/ballerina/runtime/transactions/TransactionResourceManager.java
Outdated
Show resolved
Hide resolved
...rina-runtime/src/main/java/io/ballerina/runtime/transactions/TransactionResourceManager.java
Outdated
Show resolved
Hide resolved
...rina-runtime/src/main/java/io/ballerina/runtime/transactions/TransactionResourceManager.java
Outdated
Show resolved
Hide resolved
...rina-runtime/src/main/java/io/ballerina/runtime/transactions/TransactionResourceManager.java
Outdated
Show resolved
Hide resolved
bvm/ballerina-runtime/src/main/resources/MessagesBundle.properties
Outdated
Show resolved
Hide resolved
...rina-runtime/src/main/java/io/ballerina/runtime/transactions/TransactionResourceManager.java
Outdated
Show resolved
Hide resolved
...rina-runtime/src/main/java/io/ballerina/runtime/transactions/TransactionResourceManager.java
Outdated
Show resolved
Hide resolved
Shall we add test for this change. |
Can we do this fix for 8.x as well? |
877c337
to
14805cf
Compare
@chiranSachintha Could you suggest a way to add test cases please? |
You can write an external function for the |
private static int parseTimeoutValue(Object value, int defaultValue) { | ||
if (!(value instanceof Number number)) { | ||
return defaultValue; | ||
} | ||
int timeoutValue = number.intValue(); | ||
if (timeoutValue <= 0) { | ||
return defaultValue; | ||
} | ||
return timeoutValue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static int parseTimeoutValue(Object value, int defaultValue) { | |
if (!(value instanceof Number number)) { | |
return defaultValue; | |
} | |
int timeoutValue = number.intValue(); | |
if (timeoutValue <= 0) { | |
return defaultValue; | |
} | |
return timeoutValue; | |
} | |
private static int parseTimeoutValue(Object value, int defaultValue) { | |
if (value instanceof Number number) { | |
int timeoutValue = number.intValue(); | |
if (timeoutValue > 0) { | |
return timeoutValue; | |
} | |
} | |
return defaultValue; | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, I think returning the parsed value at the bottom of the method makes it clear that the method’s primary purpose is to return a valid timeout value if possible, and only return the default value if the parsing or validation fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantage of this refactoring is this reduces the number of return statements and keeps the code concise.
...rina-runtime/src/main/java/io/ballerina/runtime/transactions/TransactionResourceManager.java
Outdated
Show resolved
Hide resolved
...rina-runtime/src/main/java/io/ballerina/runtime/transactions/TransactionResourceManager.java
Outdated
Show resolved
Hide resolved
...rina-runtime/src/main/java/io/ballerina/runtime/transactions/TransactionResourceManager.java
Outdated
Show resolved
Hide resolved
7e1c5e4
to
dba23db
Compare
Purpose
Introduces new configurations to prevent premature commits of long-running transactions before reaching the commit phase.
Related #42922
Approach
This PR introduces two new configurations values for transactions,
transactionAutoCommitTimeout
andtransactionCleanupTimeout
.ballerinai-transaction PR: ballerina-platform/module-ballerinai-transaction#549
Samples
You can change the
Config.toml
with these two new configs as below.Remarks
ballerinai-transaction PR: ballerina-platform/module-ballerinai-transaction#549
Check List