You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by yjhyjhyjh0 <gi...@git.apache.org> on 2017/09/21 12:26:11 UTC
[GitHub] nifi pull request #2166: NIFI-4395 - GenerateTableFetch can't fetch column t...
GitHub user yjhyjhyjh0 opened a pull request:
https://github.com/apache/nifi/pull/2166
NIFI-4395 - GenerateTableFetch can't fetch column type by state after…
… instance reboot
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yjhyjhyjh0/nifi NIFI-4395
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2166.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 #2166
----
commit faf6f2dfc15ef1900b6b868e19b90e39d934d07f
Author: Deon Huang <yj...@gmail.com>
Date: 2017-09-21T07:34:00Z
NIFI-4395 - GenerateTableFetch can't fetch column type by state after instance reboot
----
---
[GitHub] nifi pull request #2166: NIFI-4395 - GenerateTableFetch can't fetch column t...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2166
---
[GitHub] nifi issue #2166: NIFI-4395 - GenerateTableFetch can't fetch column type by ...
Posted by yjhyjhyjh0 <gi...@git.apache.org>.
Github user yjhyjhyjh0 commented on the issue:
https://github.com/apache/nifi/pull/2166
Thanks for the detail comment and suggestion.
That helps me a lot.
Some fix by suggestion like.
1 - put back StringmaxValueColumnNames to GenerateTableFetch to avoid NPE unit test fail.
(Originally forget to include this line)
2 - move re-cache method, calling super.setup(), before the getColumnType to provide more
readable code.
3 - Remove ternary operator because evaluateAttributeExpressions already can handle null flowfile.
Thanks
---
[GitHub] nifi issue #2166: NIFI-4395 - GenerateTableFetch can't fetch column type by ...
Posted by yjhyjhyjh0 <gi...@git.apache.org>.
Github user yjhyjhyjh0 commented on the issue:
https://github.com/apache/nifi/pull/2166
Not quite understand result from travis-ci.
Seems it stuck at ANTLR Parser AttributeExpressionParser every time?
---
[GitHub] nifi issue #2166: NIFI-4395 - GenerateTableFetch can't fetch column type by ...
Posted by paulgibeault <gi...@git.apache.org>.
Github user paulgibeault commented on the issue:
https://github.com/apache/nifi/pull/2166
@ijokarumawak Thank you for the quick review. We will get right on these changes and resubmit.
---
[GitHub] nifi issue #2166: NIFI-4395 - GenerateTableFetch can't fetch column type by ...
Posted by yjhyjhyjh0 <gi...@git.apache.org>.
Github user yjhyjhyjh0 commented on the issue:
https://github.com/apache/nifi/pull/2166
I've solved this issue by re-cache column type if needed and pass local unit and instance test.
The root cause of this issue is that originally disign didn't recognize that column type cache will be clean after instance reboot.
This cause later unable to find column type cache with existing state value and throw unhandle IllegalArgumentException.
Now if state value exist without corresponding column type cache, we will fetch and cache it.
---
[GitHub] nifi issue #2166: NIFI-4395 - GenerateTableFetch can't fetch column type by ...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2166
Hey @yjhyjhyjh0 - Don't worry about the CI builds - it has been failing for a while now. We should really find the reason why and fix it... but I didn't look into it yet. Regarding your PR, I think @ijokarumawak is going to review it when he has time for it.
---
[GitHub] nifi pull request #2166: NIFI-4395 - GenerateTableFetch can't fetch column t...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2166#discussion_r140352593
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GenerateTableFetch.java ---
@@ -243,14 +243,14 @@ public void onTrigger(final ProcessContext context, final ProcessSessionFactory
maxValueSelectColumns.add("MAX(" + colName + ") " + colName);
String maxValue = getColumnStateMaxValue(tableName, statePropertyMap, colName);
if (!StringUtils.isEmpty(maxValue)) {
- Integer type = getColumnType(tableName, colName);
+ Integer type = getColumnType(context, tableName, colName, finalFileToProcess);
--- End diff --
Probably instead of change `getColumnType`, we can add calling `setup` if `columnTypeMap.isEmpty()`, before `getColumnType` here. Which makes it more readable.
---
[GitHub] nifi pull request #2166: NIFI-4395 - GenerateTableFetch can't fetch column t...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2166#discussion_r140341776
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GenerateTableFetch.java ---
@@ -402,16 +402,20 @@ private String getColumnStateMaxValue(String tableName, Map<String, String> stat
return maxValue;
}
- private Integer getColumnType(String tableName, String colName) {
+ private Integer getColumnType(final ProcessContext context, String tableName, String colName, FlowFile flowFile) {
final String fullyQualifiedStateKey = getStateKey(tableName, colName);
Integer type = columnTypeMap.get(fullyQualifiedStateKey);
if (type == null && !isDynamicTableName) {
// If the table name is static and the fully-qualified key was not found, try just the column name
type = columnTypeMap.get(getStateKey(null, colName));
}
+ if (type == null || columnTypeMap.size() == 0) {
+ // This means column type cache is clean after instance reboot. We should re-cache column type
+ super.setup(context, false, flowFile);
--- End diff --
Calling `setup()` only updates `columnTypeMap`. The `type` variable will stay being null here. Doesn't it throw ProcessException? Shouldn't we add `type = columnTypeMap.get` after calling setup?
---
[GitHub] nifi issue #2166: NIFI-4395 - GenerateTableFetch can't fetch column type by ...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2166
The unit test doesn't pass when this PR is rebased against master, due to [NIFI-3484](https://issues.apache.org/jira/browse/NIFI-3484). I can fix this when I merge.
+1 LGTM, verified changes incorporated from @ijokarumawak 's review, built and ran (altered) unit tests, and tried with MySQL using multiple tables. Thank you for this fix! Merging to master.
---
[GitHub] nifi pull request #2166: NIFI-4395 - GenerateTableFetch can't fetch column t...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2166#discussion_r140346314
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java ---
@@ -245,7 +250,9 @@ public void setup(final ProcessContext context) {
ResultSetMetaData resultSetMetaData = resultSet.getMetaData();
int numCols = resultSetMetaData.getColumnCount();
if (numCols > 0) {
+ if (shouldCleanCache){
columnTypeMap.clear();
+ }
--- End diff --
Need indentation at the line of `columnTypeMap.clear()`.
---
[GitHub] nifi pull request #2166: NIFI-4395 - GenerateTableFetch can't fetch column t...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2166#discussion_r140349370
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java ---
@@ -221,8 +222,12 @@ protected PropertyDescriptor getSupportedDynamicPropertyDescriptor(final String
return super.customValidate(validationContext);
}
- public void setup(final ProcessContext context) {
- final String maxValueColumnNames = context.getProperty(MAX_VALUE_COLUMN_NAMES).evaluateAttributeExpressions().getValue();
+ public void setup(final ProcessContext context){
+ setup(context,true,null);
+ }
+
+ public void setup(final ProcessContext context, boolean shouldCleanCache,FlowFile flowFile) {
+ final String maxValueColumnNames = (flowFile == null) ? context.getProperty(MAX_VALUE_COLUMN_NAMES).evaluateAttributeExpressions().getValue() : context.getProperty(MAX_VALUE_COLUMN_NAMES).evaluateAttributeExpressions(flowFile).getValue();
--- End diff --
I like the idea of being defensive, but flowFile can be null with `evaluateAttributeExpressions`, so we don't need this check using ternary operator. Just passing the flowFile (possibly null) is fine, as GenerateTableFetch [existing code](https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GenerateTableFetch.java#L185) does.
---
[GitHub] nifi pull request #2166: NIFI-4395 - GenerateTableFetch can't fetch column t...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2166#discussion_r140344171
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GenerateTableFetch.java ---
@@ -153,13 +153,11 @@ public GenerateTableFetch() {
@Override
@OnScheduled
- public void setup(final ProcessContext context) {
- maxValueProperties = getDefaultMaxValueProperties(context.getProperties());
--- End diff --
We should keep this line to initialize `maxValueProperties`. I got following NPE when I restarted NiFi. Unit tests failed, too:
```
2017-09-22 04:55:35,898 WARN [Timer-Driven Process Thread-1] o.a.n.c.t.ContinuallyRunProcessorTask
java.lang.NullPointerException: null
at org.apache.nifi.processors.standard.GenerateTableFetch.onTrigger(GenerateTableFetch.java:208)
at org.apache.nifi.controller.StandardProcessorNode.onTrigger(StandardProcessorNode.java:1119)
at org.apache.nifi.controller.tasks.ContinuallyRunProcessorTask.call(ContinuallyRunProcessorTask.java:147)
at org.apache.nifi.controller.tasks.ContinuallyRunProcessorTask.call(ContinuallyRunProcessorTask.java:47)
at org.apache.nifi.controller.scheduling.TimerDrivenSchedulingAgent$1.run(TimerDrivenSchedulingAgent.java:128)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
```
---