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
QueryDatabaseTable updates #877
Conversation
@@ -17,11 +17,8 @@ | |||
package org.apache.nifi.processors.standard; | |||
|
|||
import org.apache.commons.lang3.StringUtils; | |||
import org.apache.nifi.annotation.behavior.EventDriven; | |||
import org.apache.nifi.annotation.behavior.InputRequirement; | |||
import org.apache.nifi.annotation.behavior.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our coding standards discourage the use of star imports, mind replacing this with the individual imports? Please and thanks!
Yeah, I haven't loaded in the NiFi style guide into IntelliJ; it was just trying to be helpful with the * imports. Should be all updated. All of my test cases are very positive outcome focused; not sure if you guys have any goals surrounding negative testing. |
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Set; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guy is still giving checkstyle errors. To run this locally you can use the contrib-check profile when building. I usually do this from the lowest module I'm changing, so nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors:
mvn clean install -Pcontrib-check
Your second commit is labelled NIFI-2584, but I believe it's for NIFI-2583. Would you mind re-submitting with the new commit label? Just want to make sure when the merge happens that the Jira trail is correct :) Thanks! |
I got CheckStyle working in IntelliJ this morning and all those issues popped right up. |
Everything looks good and works well, thanks! @patricker what do you think about adding fragment.count? Could be a later improvement but I think it'd be a good thing to add if not too troublesome |
I'll get it in. I didn't add any tests around those attributes, they were more of an after thought, so I'll throw a couple of those in too. |
Awesome, thanks! Looking forward to these improvements, will make QDT much more flexible and powerful |
Improvements are in, along with a few extra steps in the tests to make sure these attributes are checking out. |
+1 LGTM, thanks again for the great contribution! Merging to master |
NIFI-2582 - QueryDatabaseTable should support splitting the ResultSet into multiple flow files
NIFI-2583 - QueryDatabaseTable should allow you to specify an initial Max Value
Appropriate Unit Tests have also been developed.