You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by jeffoxenberg <gi...@git.apache.org> on 2016/07/01 18:36:58 UTC
[GitHub] nifi pull request #602: NIFI-2165: fix support for inserting timestamps into...
GitHub user jeffoxenberg opened a pull request:
https://github.com/apache/nifi/pull/602
NIFI-2165: fix support for inserting timestamps into cassandra
As per https://issues.apache.org/jira/browse/NIFI-2165, fixing support for inserting timestamps into Cassandra.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeffoxenberg/nifi NIFI-2165
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/602.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 #602
----
commit b7c1ebc546bcf554cdfa3b9761596e6e1fcdc27b
Author: jeffoxenberg <jo...@gmail.com>
Date: 2016-07-01T18:26:27Z
NIFI-2165: fix support for inserting timestamps into cassandra
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by jeffoxenberg <gi...@git.apache.org>.
Github user jeffoxenberg commented on the issue:
https://github.com/apache/nifi/pull/602
Thanks for working with me on this.
The real flow's value was double quoted, it was parsed from JSON. [Here's](https://gist.github.com/jeffoxenberg/6f12bb11b7359e10180712500cb528e7) the flow that I originally got the error from and here's a sample of the json message that nifi would consume from kafka: {"senstype": "temp3", "ts": "2016-07-01T20:52:46Z", "value": 8.72}. It also gave me an error when I used UpdateAttribute to insert the values directly instead of using variables from parsed JSON.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
You're right, we'd need some integration tests (possibly using CassandraUnit) to show this. I will verify your change then merge as-is, we can add an improvement Jira or something to add integration tests and exercise more of the type conversion logic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
+1 LGTM, merging to master
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by jeffoxenberg <gi...@git.apache.org>.
Github user jeffoxenberg commented on the issue:
https://github.com/apache/nifi/pull/602
Is the test suite actually creating a cassandra instance, creating a table, and executing the generated cql against it? I did some testing and was able to get the original code to insert by changing my c* column data type to 'text' but leaving it as 'timestamp' in Nifi, since Nifi was doing a conversion from timestamp to text. I think you'll only run into the error when the column in c* is of type timestamp, and I can't find where the test suite has that logic.
I think the test passes with the original code because after the timestamp value is set as a string [here](https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-cassandra-bundle/nifi-cassandra-processors/src/main/java/org/apache/nifi/processors/cassandra/PutCassandraQL.java#L318-L327), it gets treated as a string for the rest of the query. It wouldn't be validated as a timestamp until it actually gets inserted into a c* table with a column of type timestamp (or run through the datastax TypeCodec, which the original code bypasses for a few types), at which point the datastax driver would error.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by jeffoxenberg <gi...@git.apache.org>.
Github user jeffoxenberg commented on the issue:
https://github.com/apache/nifi/pull/602
Thanks. I think I got it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
@jeffoxenberg After your fix is applied, I tried a QueryCassandra to get the values out of the table, and I'm getting a similar codec error then. Should we edit the Jira to fix both cases, or do you want to handle just the PutCassandraQL issue here, and do the QueryCassandra one separately?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
An example could be another test just like that one with cql.args.10.value set to "I'm not a timestamp" or a timestamp not in ISO-8601 format like "07.01.2016". Not saying the operation should succeed, but should be handled appropriately (error logged, flow file transferred to failure, e.g.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by jeffoxenberg <gi...@git.apache.org>.
Github user jeffoxenberg commented on the issue:
https://github.com/apache/nifi/pull/602
No problem. I think I added a test for a good value here: `https://github.com/jeffoxenberg/nifi/commit/335e4f08a60989909d2d152f426fa5d46dec9a1b`.
Could you give me an example of writing a test for a bad value? Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
I cherry-picked in the commits that added unit tests, to show that they would fail without your fix, however they pass. Any idea how the tests differ from the error that spawned the Jira?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
@jeffoxenberg mind squashing your commits into one? Please and thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
Looks good, mind adding unit test(s) to try various good and bad values?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by jeffoxenberg <gi...@git.apache.org>.
Github user jeffoxenberg commented on the issue:
https://github.com/apache/nifi/pull/602
Squashed my commits. I'll look into integration testing as well. I'll try to handle the QueryCassandra one separately as I'll be busy for the next fewof days with other things.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
Can you put your template in a Gist? I'll give it a try too
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #602: NIFI-2165: fix support for inserting timestamps into...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/602
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
Were there quotes (single or double) in the real flow's value?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #602: NIFI-2165: fix support for inserting timestamps into cassan...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/602
Ok sounds good. I gave the Integration Test thing (and the QueryCassandra -- actually in AbstractCassandraProcessor) fix a try, the ITs don't work (yet) but feel free to take a look: https://github.com/mattyb149/nifi/tree/cassandra_time
I'll write up the Jira for QueryCassandra, and if I get a chance to test the fix I will submit the PR myself and you can review if you like :)
Will do a final look-around then merge, thanks again!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---