You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by viswaug <gi...@git.apache.org> on 2018/10/26 19:58:35 UTC
[GitHub] nifi pull request #3113: making the database connection autocommit value a c...
GitHub user viswaug opened a pull request:
https://github.com/apache/nifi/pull/3113
making the database connection autocommit value a configurable property
Currently, the PutSQL processor sets the database connection auto commit to false before executing the SQL statement(s) and this is hardcoded in. This commit makes the auto commit value being set to be configurable.
https://issues.apache.org/jira/projects/NIFI/issues/NIFI-5724?filter=allissues
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Snagajob/nifi configurable_autocommit_putsql
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/3113.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3113
----
commit f63314c3db909d99acce1a071faa889a9cceca36
Author: Vish Uma <vi...@...>
Date: 2018-10-26T19:32:46Z
making the database session autocommit value a configurable property
----
---
[GitHub] nifi pull request #3113: NIFI-5724 making the database connection autocommit...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3113#discussion_r231743426
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSQL.java ---
@@ -134,6 +134,14 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ static final PropertyDescriptor AUTO_COMMIT = new PropertyDescriptor.Builder()
+ .name("database-session-autocommit")
+ .displayName("Database session autocommit value")
+ .description("The autocommit mode to set on the database connection being used.")
+ .allowableValues("true", "false")
+ .defaultValue("false")
+ .build();
--- End diff --
@viswaug Thanks for the snowflake doc link and explanation. Look forward to see new commits for further review. In order to update your PR, you just need to:
1. Make code changes
2. Add the changed files for a commit: `git add <file>` or `git add -A` (all changed files)
3. Commit with some comments: `git commit`, this makes a commit to your local branch
4. Push the commit to your remote branch: `git push origin configurable_autocommit_putsql`. This command adds the new commit to this PR.
---
[GitHub] nifi pull request #3113: NIFI-5724 making the database connection autocommit...
Posted by viswaug <gi...@git.apache.org>.
Github user viswaug commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3113#discussion_r231747175
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSQL.java ---
@@ -134,6 +134,14 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ static final PropertyDescriptor AUTO_COMMIT = new PropertyDescriptor.Builder()
+ .name("database-session-autocommit")
+ .displayName("Database session autocommit value")
+ .description("The autocommit mode to set on the database connection being used.")
+ .allowableValues("true", "false")
+ .defaultValue("false")
+ .build();
--- End diff --
@ijokarumawak that git(hub) part was much easier than i expected it to be... i checked in my changes... let me know if you need anything tweaked or changed 🤞
---
[GitHub] nifi pull request #3113: NIFI-5724 making the database connection autocommit...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3113#discussion_r232159628
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSQL.java ---
@@ -189,13 +201,40 @@
properties.add(CONNECTION_POOL);
properties.add(SQL_STATEMENT);
properties.add(SUPPORT_TRANSACTIONS);
+ properties.add(AUTO_COMMIT);
properties.add(TRANSACTION_TIMEOUT);
properties.add(BATCH_SIZE);
properties.add(OBTAIN_GENERATED_KEYS);
properties.add(RollbackOnFailure.ROLLBACK_ON_FAILURE);
return properties;
}
+ @Override
+ protected final Collection<ValidationResult> customValidate(ValidationContext context) {
+ final Collection<ValidationResult> results = new ArrayList<>();
+ final String support_transactions = context.getProperty(SUPPORT_TRANSACTIONS).getValue();
+ final String rollback_on_failure = context.getProperty(RollbackOnFailure.ROLLBACK_ON_FAILURE).getValue();
+ final String auto_commit = context.getProperty(AUTO_COMMIT).getValue();
+
+ if(auto_commit.equalsIgnoreCase("true")) {
+ if(support_transactions.equalsIgnoreCase("true")) {
+ results.add(new ValidationResult.Builder()
+ .subject(SUPPORT_TRANSACTIONS.getDisplayName())
+ .explanation(format("'%s' cannot be set to 'true' when '%s' is also set to 'true'."
+ + "Transactions for batch updates cannot be supported when auto commit is set to 'true'", SUPPORT_TRANSACTIONS.getDisplayName(), AUTO_COMMIT.getDisplayName()))
+ .build());
+ }
+ if(rollback_on_failure.equalsIgnoreCase("true")) {
+ results.add(new ValidationResult.Builder()
+ .subject(RollbackOnFailure.ROLLBACK_ON_FAILURE.getDisplayName())
+ .explanation(format("'%s' cannot be set to 'true' when '%s' is also set to 'true'."
+ + "Transaction rollbacks for batch updates cannot be supported when auto commit is set to 'true'", RollbackOnFailure.ROLLBACK_ON_FAILURE.getDisplayName(), AUTO_COMMIT.getDisplayName()))
--- End diff --
This line causes following check-style error. Please build with `-Pcontrib-check` to confirm styles locally.
```
src/main/java/org/apache/nifi/processors/standard/PutSQL.java:[231] (sizes) LineLength: Line is longer than 200 characters (found 217).
```
---
[GitHub] nifi pull request #3113: NIFI-5724 making the database connection autocommit...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3113#discussion_r229960433
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSQL.java ---
@@ -134,6 +134,14 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ static final PropertyDescriptor AUTO_COMMIT = new PropertyDescriptor.Builder()
+ .name("database-session-autocommit")
+ .displayName("Database session autocommit value")
+ .description("The autocommit mode to set on the database connection being used.")
+ .allowableValues("true", "false")
+ .defaultValue("false")
+ .build();
--- End diff --
@viswaug That approach may work, too. However, I prefer exposing auto-commit property and let user to enable it explicitly. And protect existing logics work as expected by adding following custom validation:
- if auto-commit is enabled:
- `Support Fragmented Transactions` should be false
- && `Rollback On Failure` should be false
This way even if we forgot about other conditions that require auto-commit, user can disable auto-commit to work-around. PutSQL has been there for long time and used by so many flows. We can not introduce any degrading issue by supporting auto-commit.
BTW, the description of Snowflake database issue is not clear enough to me on how auto-commit setting relates to the issue.
> This is causing an issue with the snowflake DB where abruptly disconnected sessions do not release the locks they have taken.
Do you have any existing Snowflake issue or resource that we can refer? Or if not have you consider reporting the issue to Snowflake project?
And also, if you have any use-case (other than avoiding Snowflake issue) where auto-commit is preferable, please add such to auto-commit property description.
---
[GitHub] nifi pull request #3113: NIFI-5724 making the database connection autocommit...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/3113
---
[GitHub] nifi pull request #3113: NIFI-5724 making the database connection autocommit...
Posted by viswaug <gi...@git.apache.org>.
Github user viswaug commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3113#discussion_r229756472
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSQL.java ---
@@ -134,6 +134,14 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ static final PropertyDescriptor AUTO_COMMIT = new PropertyDescriptor.Builder()
+ .name("database-session-autocommit")
+ .displayName("Database session autocommit value")
+ .description("The autocommit mode to set on the database connection being used.")
+ .allowableValues("true", "false")
+ .defaultValue("false")
+ .build();
--- End diff --
@ijokarumawak gotcha. how about i remove the property i just added. and set autocommit to true when the SUPPORT_TRANSACTIONS property value is set to false?
---
[GitHub] nifi pull request #3113: NIFI-5724 making the database connection autocommit...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3113#discussion_r232166089
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSQL.java ---
@@ -134,6 +138,14 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ static final PropertyDescriptor AUTO_COMMIT = new PropertyDescriptor.Builder()
+ .name("database-session-autocommit")
+ .displayName("Database session autocommit value")
--- End diff --
I'm going to change this displayName to "Database Session AutoCommit" to align capitalization with other properties.
---
[GitHub] nifi pull request #3113: NIFI-5724 making the database connection autocommit...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3113#discussion_r228789994
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSQL.java ---
@@ -134,6 +134,14 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ static final PropertyDescriptor AUTO_COMMIT = new PropertyDescriptor.Builder()
+ .name("database-session-autocommit")
+ .displayName("Database session autocommit value")
+ .description("The autocommit mode to set on the database connection being used.")
+ .allowableValues("true", "false")
+ .defaultValue("false")
+ .build();
--- End diff --
@viswaug IIRC disable auto-commit is required to support "Support Fragmented Transactions" and/or "Rollback On Failure". We need to implement `customValidate` method, too, if we're going to make auto-commit configurable. If auto-commit is enabled, the processor should not allow using "Support Fragmented Transactions" and/or "Rollback On Failure".
---
[GitHub] nifi pull request #3113: NIFI-5724 making the database connection autocommit...
Posted by viswaug <gi...@git.apache.org>.
Github user viswaug commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3113#discussion_r231638762
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSQL.java ---
@@ -134,6 +134,14 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ static final PropertyDescriptor AUTO_COMMIT = new PropertyDescriptor.Builder()
+ .name("database-session-autocommit")
+ .displayName("Database session autocommit value")
+ .description("The autocommit mode to set on the database connection being used.")
+ .allowableValues("true", "false")
+ .defaultValue("false")
+ .build();
--- End diff --
@ijokarumawak Here is the snowflake documentation that refers to the locks being held for ever during abruptly disconnected sessions.
https://docs.snowflake.net/manuals/sql-reference/transactions.html#aborting-transactions
I have also confirmed this with the snowflake support ticket and the resolution suggested was to set the autocommit value to true.
> I did reviewed the query id thanks for providing it. The query is associated to session id 2392474452590 and from the session id we found that there was an alter session set autocommit=false was executed (query id for your reference 10d44ff5-69a4-4d8c-91c3-a206c4a126b8) and after that there was no commit executed explicitly. This lead to the open transactions in the session and hence there was a lock created.
> By default autocommit is true and, once all the transaction is completed, it gets automatically committed. The commit query will not be visible in the history as this is a background task. For best practices, we recommend to manually commit the transactions if the session has set to autocommit=false .
> Yes, there is a difference between a session being "closed" versus "terminated abruptly". Session being "closed" implies to those sessions which is closed manually after all the transactions is completed . Session being "terminated abruptly" implies to those sessions which terminates due to various reason like network issues, system outage ..etc.
I am done making the changes you had requested. I will have a PR out soon. I just need to hone my git skills to combine these commits and send a PR ... still figuring that part out ...
---