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-3335: Add initial.maxvalue support to GenerateTableFetch #2039
Conversation
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.
Hey @mattyb149, code LGTM, just left one comment.
@@ -198,6 +201,25 @@ public void onTrigger(final ProcessContext context, final ProcessSessionFactory | |||
// set as the current state map (after the session has been committed) | |||
final Map<String, String> statePropertyMap = new HashMap<>(stateMap.toMap()); | |||
|
|||
// If an initial max value for column(s) has been specified using properties, and this column is not in the state manager, sync them to the state property map | |||
final Map<String,String> maxValueProperties = getDefaultMaxValueProperties(context.getProperties()); |
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.
Would it make sense to do it in the @OnScheduled
method of the Abstract class? You could have the same code in common for QueryDatabaseTable and GenerateTableFetch. Besides not sure it needs to be done each time the processor is triggered? Thoughts?
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.
Yeah that's a better idea, should've been done in the original PR but I missed it during my review and just cut-pasted it back to the abstract class to share.
I won't be able to move all that common code block as it needs the table name which can come from a flow file. But I can call getDefaultMaxValueProperties() at schedule-time vs trigger time.
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.
+1, squashing and merging to master, thanks @mattyb149
Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com> This closes apache#2039.
Moved the common code back to the abstract base class
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 NIFI-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:
For documentation related changes:
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.