MINIFI-1013 Used soci library. by am-c-p-p · Pull Request #656 · apache/nifi-minifi-cpp

am-c-p-p · 2019-10-01T09:31:49Z

Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file?
If applicable, have you updated the NOTICE file?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

arpadboda

Overall looks to be a nice work, thanks!
Questionable points for me are:
-Linux compilation
-Parallel onTrigger calls, connection reuse

Some minor comments as well, but those are low hanging fruits.

extensions/sql/CMakeLists.txt

extensions/sql/cmake/FindODBC.cmake

extensions/sql/data/DatabaseConnectors.h

extensions/sql/data/JSONSQLWriter.h

extensions/sql/processors/ExecuteSQL.cpp

extensions/sql/win/Odbc.cpp

bakaid

Please see my inline comments, some are minor, some are important.

After the build fixes I have been able to use a mysql ODBC driver on macOS to connect to a mysql instance and the processor seems to work alright, there is some good stuff in here!

Attached are all the fixes in a patch file (they are also added as inline comments, but this might be easier to use):
sql-extension-patch.txt

extensions/sql/processors/ExecuteSQL.h

extensions/sql/services/DatabaseService.h

extensions/sql/CMakeLists.txt

extensions/sql/services/ODBCConnector.h

extensions/sql/data/JSONSQLWriter.cpp

extensions/sql/CMakeLists.txt

phrocker

I don't know if the attribution of these commits is correct. That should be addressed before merge to be safe. Feel free to correct me @am-c-p-p

am-c-p-p · 2019-10-08T09:19:29Z

@phrocker Fixed attribution.

am-c-p-p · 2019-11-28T10:37:37Z

Added PutSQL implementation.

arpadboda · 2019-12-04T12:13:08Z

extensions/sql/processors/PutSQL.cpp

+    if (connection) {
+      const auto dbSession = connection->getSession();
+
+      try {


What are we supposed to do here?
Given the name (PutSQL) I would expect content of flowfiles to be put to the DB.
Although I don't see any sign of that in the code, we only execute the statements provided by the user.

Have a success relationship hardly makes sense, but I think we should have a failure and work as:
-Send to success in case we could properly execute the put.
-Send to failure in case SQL operation failed. (Optional rollback here)

Please take a look at PutSQL in NiFi:
https://nifi.apache.org/docs/nifi-docs/components/org.apache.nifi/nifi-standard-nar/1.6.0/org.apache.nifi.processors.standard.PutSQL/

Especially the flowfile-dependent attributes it reads.

arpadboda

Far from getting to the end.
Added some comments. Please also address the older ones in case they are still valid.

extensions/sql/data/Utils.cpp

extensions/sql/data/JSONSQLWriter.cpp

extensions/sql/data/MaxCollector.h

extensions/sql/data/SQLWriter.h

extensions/sql/processors/ExecuteSQL.cpp

extensions/sql/processors/PutSQL.cpp

extensions/sql/processors/ExecuteSQL.cpp

extensions/sql/processors/QueryDatabaseTable.cpp

…ixes for soci.

bakaid

Releasing what I have so far. I have marked the most important comments with (!). Did not yet review QueryDatabaseTable, and did not test anything so far.

extensions/sql/SQLLoader.h

extensions/sql/services/DatabaseService.cpp

extensions/sql/processors/PutSQL.cpp

extensions/sql/processors/ExecuteSQL.cpp

bakaid · 2019-12-18T15:51:07Z

extensions/sql/processors/PutSQL.cpp

@@ -0,0 +1,134 @@
+/**
+ * @file PutSQL.cpp


(!) Is this processor finished? It seems to be equivalent to ExecuteSQL, as it gets a bunch of statements as a Property. With PutSQL the statement should be contained in the FlowFile content and substitutions should be made based on FF-attributes: https://nifi.apache.org/docs/nifi-docs/components/org.apache.nifi/nifi-standard-nar/1.10.0/org.apache.nifi.processors.standard.PutSQL/

extensions/sql/win/Odbc.cpp

bakaid

Reviewing and testing - not nearly finished, but releasing what I have before going to lunch.

extensions/sql/processors/ExecuteSQL.cpp

extensions/sql/data/JSONSQLWriter.cpp

bakaid

Releasing the next batch.

extensions/sql/processors/QueryDatabaseTable.cpp

CMakeLists.txt

extensions/sql/processors/QueryDatabaseTable.cpp

…Table

…ts like Avro in addition to JSON.

…_build_vs.bat.

…inifi-cpp into minifi-1013-soci

arpadboda · 2020-02-06T12:35:53Z

@am-c-p-p do we need this PR?
Please close otherwise.

am-c-p-p · 2020-02-06T20:48:22Z

@arpadboda Yes, we need it. It is reviewed by @bakaid and pending testing before merging to apache.

bakaid · 2020-02-10T12:55:58Z

@arpadboda @am-c-p-p The diff on this PR currently looks like this:

My guess is that a rebased branch has been merged, ending up recreating most of the history.
@am-c-p-p please fix this, rebase to the latest master and squash the whole PR into one commit, after that I will do a final verification.

am-c-p-p · 2020-02-11T08:24:13Z

@bakaid @arpadboda Instead of this PR please review new #732.
Current one can be used as a reference, I'll delete it after new one is merged into apache.

bakaid · 2020-04-08T14:03:27Z

@amarmer since #732 is merged, I assume we can close this, is that right?

bakaid · 2020-04-15T11:20:29Z

@amarmer Closing this, feel free to reopen if it is relevant after all.

arpadboda mentioned this pull request Oct 3, 2019

MINIFICPP-1013 ExecuteSQL Partially implemented #644

Closed

8 tasks

arpadboda reviewed Oct 3, 2019

View reviewed changes

bakaid reviewed Oct 4, 2019

View reviewed changes

phrocker requested changes Oct 6, 2019

View reviewed changes

am-c-p-p force-pushed the minifi-1013-soci branch from a43582a to 7dc002e Compare October 8, 2019 09:17

arpadboda reviewed Dec 4, 2019

View reviewed changes

arpadboda reviewed Dec 16, 2019

View reviewed changes

phrocker and others added 14 commits December 18, 2019 00:01

MINIFI-1013 Used soci library.

5629024

MINIFI-1013 Removed commented code.

eb6ec18

MINIFI-1013 Substituted Tabs with spaces.

db5b5e7

MINIFICPP-1013 Code review.

5584b90

MINIFICPP-1013 Code review.

08ed2cb

MINIFICPP-1013 Code review.

1378cb4

MINIFICPP-1013 Implemented PutSQL.

cbecd82

MINIFI-1013 Code review.

516df7e

minificpp-1013 Implemented QueryDatabaseTable.

7a999f3

minificpp-1013 Implemented QueryDatabaseTable.

825c311

minificpp-1013 QueryDatabaseTable, removed debugging output.

73fb957

minificpp-1013 Code review.

e41f708

minificpp-1013 Code review.

08b8f5b

minificpp-1013 Merged from apache mactrer and applied @bakaid build f…

a7b80a2

…ixes for soci.

am-c-p-p force-pushed the minifi-1013-soci branch from 1fb58fc to a7b80a2 Compare December 18, 2019 08:14

minificpp-1013 Code review.

878b09b

bakaid suggested changes Dec 18, 2019

View reviewed changes

amarmer added 2 commits December 19, 2019 01:06

minificpp-1013 Code review.

bdea835

minificpp-1013 Code review.

6048a5c

bakaid reviewed Dec 19, 2019

View reviewed changes

bakaid suggested changes Dec 19, 2019

View reviewed changes

extensions/sql/processors/QueryDatabaseTable.cpp Show resolved Hide resolved

extensions/sql/processors/QueryDatabaseTable.cpp Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

extensions/sql/processors/QueryDatabaseTable.cpp Outdated Show resolved Hide resolved

amarmer and others added 22 commits January 22, 2020 10:47

minificpp-1013 Code review.

eb359fe

minicpp-1013 Code review.

5ba7254

minicpp-1013 Code review.

2ebab81

minicpp-1013 Code review.

ec42a68

minicpp-1013 Removed debugging.

dc6323a

minicpp-1013 Code review.

c096e63

MINIFICPP-1013 - Added initial maximum value support to QueryDatabase…

575fd36

…Table

MINIFICPP-1013 Code review - refactored to support other output forma…

3e32735

…ts like Avro in addition to JSON.

MINIFICPP-1013 Code review - added common base class SQLProcessor.

4d0fe30

MINIFICPP-1013 Very small refactoring.

7f1a7a5

MINIFICPP-1013 Code review - added ENABLE_SQL in bootstrap.sh and win…

c59bf8f

…_build_vs.bat.

MINIFICPP-1013 Code review - fixed reconnect on each onTrigger call.

f59936e

MINIFICPP-1013 Refactoring connection drop.

b9ba731

MINIFICPP-1013 Very small refactoring.

9912815

MINIFICPP-1013 Refactoring.

f631901

MINIFICPP-1013 Code review.

84d2ca6

MINIFICPP-1013 Code review.

96f4710

MINIFICPP-1013 Formatting.

3541aeb

MINIFICPP-1013 - Fix *nix build

214f7c2

MINIFICPP-1013 Code review.

a631803

MINIFICPP-1013 Code review.

1c82aaa

Merge branch 'minifi-1013-soci' of https://github.com/am-c-p-p/nifi-m…

0f489fe

…inifi-cpp into minifi-1013-soci

am-c-p-p mentioned this pull request Feb 11, 2020

MINIFICPP-1013 #732

Closed

8 tasks

bakaid closed this Apr 15, 2020

Comments

Conversation

am-c-p-p commented Oct 1, 2019

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

arpadboda left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bakaid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phrocker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

am-c-p-p commented Oct 8, 2019

Uh oh!

am-c-p-p commented Nov 28, 2019

Uh oh!

arpadboda Dec 4, 2019

Choose a reason for hiding this comment

Uh oh!

arpadboda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bakaid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bakaid Dec 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bakaid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bakaid left a comment

arpadboda left a comment •

edited

Loading

phrocker left a comment •

edited

Loading

am-c-p-p commented Feb 6, 2020 •

edited

Loading

am-c-p-p commented Feb 11, 2020 •

edited

Loading