[🍒] Add TRANSACTION_ISOLATION_LEVEL config in cloudsql-postgres and cloudsql-mysql plugins for release/1.12#676
Conversation
…sql-mysql plugins for develop fix: updated widget and docs fix: typos Added a constant for default isolation level
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces support for configuring the transaction isolation level in the CloudSQL MySQL and CloudSQL PostgreSQL plugins, updating their respective documentation, widget configurations, and source configurations. The review feedback highlights several critical improvements: first, the getTransactionIsolationLevel() method in both source classes should delegate to the connection configuration when useConnection is enabled; second, the transactionIsolationLevel fields should be annotated with @Macro to support dynamic runtime configuration; and finally, the widget JSON files should be updated to include transactionIsolationLevel in the showConnectionProperties filter to hide the setting when a connection is utilized.
| @Override | ||
| public String getTransactionIsolationLevel() { | ||
| return transactionIsolationLevel != null ? transactionIsolationLevel : DEFAULT_TRANSACTION_ISOLATION_LEVEL; | ||
| } |
There was a problem hiding this comment.
When useConnection is enabled, the source should inherit the transaction isolation level configured in the connection. Currently, getTransactionIsolationLevel() completely ignores the connection's configuration and only checks the stage-level property, which means any setting configured in the Connection is ignored.
Please update the method to delegate to the connection config when useConnection is true.
@Override
public String getTransactionIsolationLevel() {
if (useConnection != null && useConnection && connection != null) {
return connection.getTransactionIsolationLevel() != null ? connection.getTransactionIsolationLevel() : DEFAULT_TRANSACTION_ISOLATION_LEVEL;
}
return transactionIsolationLevel != null ? transactionIsolationLevel : DEFAULT_TRANSACTION_ISOLATION_LEVEL;
}| @Override | ||
| public String getTransactionIsolationLevel() { | ||
| return transactionIsolationLevel != null ? transactionIsolationLevel : DEFAULT_TRANSACTION_ISOLATION_LEVEL; | ||
| } |
There was a problem hiding this comment.
When useConnection is enabled, the source should inherit the transaction isolation level configured in the connection. Currently, getTransactionIsolationLevel() completely ignores the connection's configuration and only checks the stage-level property, which means any setting configured in the Connection is ignored.
Please update the method to delegate to the connection config when useConnection is true.
@Override
public String getTransactionIsolationLevel() {
if (useConnection != null && useConnection && connection != null) {
return connection.getTransactionIsolationLevel() != null ? connection.getTransactionIsolationLevel() : DEFAULT_TRANSACTION_ISOLATION_LEVEL;
}
return transactionIsolationLevel != null ? transactionIsolationLevel : DEFAULT_TRANSACTION_ISOLATION_LEVEL;
}| @Name(TRANSACTION_ISOLATION_LEVEL) | ||
| @Description("Transaction isolation level for queries run by this source.") | ||
| @Nullable | ||
| private String transactionIsolationLevel; |
There was a problem hiding this comment.
The transactionIsolationLevel field is missing the @Macro annotation. Adding @Macro allows users to dynamically configure the transaction isolation level using pipeline arguments or macros at runtime.
| @Name(TRANSACTION_ISOLATION_LEVEL) | |
| @Description("Transaction isolation level for queries run by this source.") | |
| @Nullable | |
| private String transactionIsolationLevel; | |
| @Name(TRANSACTION_ISOLATION_LEVEL) | |
| @Description("Transaction isolation level for queries run by this source.") | |
| @Nullable | |
| @Macro | |
| private String transactionIsolationLevel; |
| @Name(TRANSACTION_ISOLATION_LEVEL) | ||
| @Description("Transaction isolation level for queries run by this source.") | ||
| @Nullable | ||
| private String transactionIsolationLevel; |
There was a problem hiding this comment.
The transactionIsolationLevel field is missing the @Macro annotation. Adding @Macro allows users to dynamically configure the transaction isolation level using pipeline arguments or macros at runtime.
| @Name(TRANSACTION_ISOLATION_LEVEL) | |
| @Description("Transaction isolation level for queries run by this source.") | |
| @Nullable | |
| private String transactionIsolationLevel; | |
| @Name(TRANSACTION_ISOLATION_LEVEL) | |
| @Description("Transaction isolation level for queries run by this source.") | |
| @Nullable | |
| @Macro | |
| private String transactionIsolationLevel; |
| { | ||
| "widget-type": "select", | ||
| "label": "Transaction Isolation Level", | ||
| "name": "transactionIsolationLevel", | ||
| "widget-attributes": { | ||
| "default": "TRANSACTION_REPEATABLE_READ", | ||
| "values": [ | ||
| "TRANSACTION_REPEATABLE_READ", | ||
| "TRANSACTION_READ_UNCOMMITTED", | ||
| "TRANSACTION_READ_COMMITTED", | ||
| "TRANSACTION_SERIALIZABLE" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
The transactionIsolationLevel property is not included in the showConnectionProperties filter's show list. This means that when useConnection is set to true, the transactionIsolationLevel dropdown will still be visible in the Source stage UI, which is confusing since the setting should be inherited from the Connection.
Please update the showConnectionProperties filter (further down in this file) to include "transactionIsolationLevel" in its show list so that it is hidden when a connection is used.
| { | ||
| "widget-type": "select", | ||
| "label": "Transaction Isolation Level", | ||
| "name": "transactionIsolationLevel", | ||
| "widget-attributes": { | ||
| "default": "TRANSACTION_READ_COMMITTED", | ||
| "values": [ | ||
| "TRANSACTION_READ_COMMITTED", | ||
| "TRANSACTION_REPEATABLE_READ", | ||
| "TRANSACTION_SERIALIZABLE" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
The transactionIsolationLevel property is not included in the showConnectionProperties filter's show list. This means that when useConnection is set to true, the transactionIsolationLevel dropdown will still be visible in the Source stage UI, which is confusing since the setting should be inherited from the Connection.
Please update the showConnectionProperties filter (further down in this file) to include "transactionIsolationLevel" in its show list so that it is hidden when a connection is used.
[🍒]
🍒 [cherrypick]
PR : (#675)
Description:
Add TRANSACTION_ISOLATION_LEVEL config in cloudsql-postgres and cloudsql-mysql plugins