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

NIFI-3611: Added ability to set Transaction Isolation Level on Database connections for QueryDatabaseTable processor #3248

Closed
wants to merge 2 commits into from

Conversation

erichanson5
Copy link
Contributor

@erichanson5 erichanson5 commented Jan 7, 2019

NIFI-3611: Added ability to set Transaction Isolation Level on Database connections for QueryDatabaseTable processor

@erichanson5 erichanson5 changed the title NIFI-6311: Added ability to set Transaction Isolation Level on Database connections for QueryDatabaseTable processor NIFI-3611: Added ability to set Transaction Isolation Level on Database connections for QueryDatabaseTable processor Jan 7, 2019
Copy link
Member

@ijokarumawak ijokarumawak left a comment

Choose a reason for hiding this comment

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

Thanks @erichanson5 for your contribution!
I concern about changing default behavior, so let's make the transaction isolation level optional.

@ijokarumawak
Copy link
Member

Hi @erichanson5 Thanks for trying to incorporate the review comments. Now this PR has other unnecessary commits in it and the it looks not compiling well. Would you clean the commits?

I don't use merge when updating the local brach with the latest master, instead I use rebase.

If I'd recover from this state, I may use following steps:

  1. Rename the current branch to different name, to clean-up the branch.
    e.g. git branch -m NIFI-3611-bk

  2. Update the master branch to the latest. e.g.

    $ git checkout master
    # the name 'upstream' may be different, it's pointing github.com/apache/nifi.git
    $ git pull upstream master
    
  3. Create new branch from the latest master.
    e.g. git checkout -b NIFI-3611

  4. Port the 1st commit from the old branch.
    e.g. git cherry-pick 27d91507a00e095e212c1c96856806f53e868b82

    At this point, your new NIFI-3611 branch is synched with the latest master and has the 1st commit.

  5. Apply the changes you wanted to make at the last commit. I recommend applying changes manually instead of cherry-picking here. At this point, your branch has the updates to incorporate review comments.

  6. Push it to update this PR.
    e.g. git push -f origin NIFI-3611
    You need -f option to forcefully push the new commits

Hope this helps.

@erichanson5
Copy link
Contributor Author

@ijokarumawak thanks for the assistance. I updated the branch

@patricker
Copy link
Contributor

This sounds like a good general feature for a few SQL processors, like ExecuteSQL, maybe GenerateTableFetch.

I'm OK with limiting the scope to QDB for now, but what about putting the allowed values and property definition in AbstractDatabaseFetchProcessor so it's ready for use in other processors?

@erichanson5
Copy link
Contributor Author

Thanks @patricker, I'll create another Jira ticket for that and work on it

@ijokarumawak
Copy link
Member

The update LGTM, +1. Merging to master. Thanks @erichanson5!

@asfgit asfgit closed this in 4ef2251 Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants