Add TRANSACTION_ISOLATION_LEVEL config in cloudsql-postgres and cloudsql-mysql plugins for develop#675
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the 'Transaction Isolation Level' configuration option for CloudSQL MySQL and CloudSQL PostgreSQL plugins, updating their batch sinks, batch sources, connectors, and documentation. The review feedback highlights several improvement opportunities, including changing the visibility of the new configuration field in CloudSQLMySQLSource from public to private for better encapsulation, removing redundant lines, correcting typos (such as 'databse'), and ensuring consistent colon usage in the documentation files.
| @Name(TRANSACTION_ISOLATION_LEVEL) | ||
| @Description("Transaction isolation level for queries run by this source.") | ||
| @Nullable | ||
| public String transactionIsolationLevel; |
There was a problem hiding this comment.
The field transactionIsolationLevel is declared as public, whereas other configuration fields in this class (and the corresponding field in CloudSQLPostgreSQLSource) are declared as private. For consistency and better encapsulation, please change its visibility to private.
| public String transactionIsolationLevel; | |
| private String transactionIsolationLevel; |
| **Transaction Isolation Level:** Transaction isolation level for queries run by this sink. | ||
| **Transaction Isolation Level** The transaction isolation level of the database connection |
There was a problem hiding this comment.
These two lines are redundant and contain a missing colon. They can be merged into a single, clean description.
| **Transaction Isolation Level:** Transaction isolation level for queries run by this sink. | |
| **Transaction Isolation Level** The transaction isolation level of the database connection | |
| **Transaction Isolation Level:** The transaction isolation level of the database connection. |
|
|
||
| **Password:** Password to use to connect to the specified database. | ||
|
|
||
| **Transaction Isolation Level** The transaction isolation level of the database connection |
There was a problem hiding this comment.
Add a colon after the parameter name for consistency with other documented properties (e.g., **Password:**).
| **Transaction Isolation Level** The transaction isolation level of the database connection | |
| **Transaction Isolation Level:** The transaction isolation level of the database connection. |
|
|
||
| **Password:** Password to use to connect to the specified database. | ||
|
|
||
| **Transaction Isolation Level** The transaction isolation level of the database connection |
There was a problem hiding this comment.
Add a colon after the parameter name for consistency with other documented properties (e.g., **Password:**).
| **Transaction Isolation Level** The transaction isolation level of the database connection | |
| **Transaction Isolation Level:** The transaction isolation level of the database connection. |
| **Transaction Isolation Level:** Transaction isolation level for queries run by this sink. | ||
| **Transaction Isolation Level** The transaction isolation level of the databse connection |
There was a problem hiding this comment.
These two lines are redundant and contain a typo ('databse' instead of 'database'). They can be merged into a single, clean description.
| **Transaction Isolation Level:** Transaction isolation level for queries run by this sink. | |
| **Transaction Isolation Level** The transaction isolation level of the databse connection | |
| **Transaction Isolation Level:** The transaction isolation level of the database connection. |
|
|
||
| **Password:** Password to use to connect to the specified database. | ||
|
|
||
| **Transaction Isolation Level** The transaction isolation level of the databse connection |
There was a problem hiding this comment.
|
|
||
| **Password:** Password to use to connect to the specified database. | ||
|
|
||
| **Transaction Isolation Level** The transaction isolation level of the databse connection |
There was a problem hiding this comment.
…sql-mysql plugins for develop
bfd80f6 to
552c462
Compare
| "label": "Transaction Isolation Level", | ||
| "name": "transactionIsolationLevel", | ||
| "widget-attributes": { | ||
| "default": "TRANSACTION_READ_COMMITTED", |
There was a problem hiding this comment.
This should not change, keep it as it is. and keep the documentation also same
Add TRANSACTION_ISOLATION_LEVEL config in cloudsql-postgres and cloudsql-mysql plugins