You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by markap14 <gi...@git.apache.org> on 2016/07/30 01:21:05 UTC
[GitHub] nifi pull request #752: NIFI-2291: Correct the Content URI for 1.0.0 REST AP...
GitHub user markap14 opened a pull request:
https://github.com/apache/nifi/pull/752
NIFI-2291: Correct the Content URI for 1.0.0 REST API; added cluster \u2026
\u2026node identifier & whether or not clustered to ReportingContext so that the Reporting Task could make use of it
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/markap14/nifi NIFI-2291
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/752.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 #752
----
commit 998a5be3b1fef77f1cb189e9b88e158656433d11
Author: Mark Payne <ma...@hotmail.com>
Date: 2016-07-30T01:17:47Z
NIFI-2291: Correct the Content URI for 1.0.0 REST API; added cluster node identifier & whether or not clustered to ReportingContext so that the Reporting Task could make use of 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 #752: NIFI-2291: Correct the Content URI for 1.0.0 REST API; adde...
Posted by YolandaMDavis <gi...@git.apache.org>.
Github user YolandaMDavis commented on the issue:
https://github.com/apache/nifi/pull/752
@markap14 thanks for the update Mark! I'll take a look. And yes completely agreed, very odd.
---
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 #752: NIFI-2291: Correct the Content URI for 1.0.0 REST API; adde...
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi/pull/752
@YolandaMDavis @markap14 I ran into the same error with the logging... Yolanda's suggestion sounds good to me
---
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 #752: NIFI-2291: Correct the Content URI for 1.0.0 REST API; adde...
Posted by YolandaMDavis <gi...@git.apache.org>.
Github user YolandaMDavis commented on the issue:
https://github.com/apache/nifi/pull/752
@markap14 picking this up for review
---
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 #752: NIFI-2291: Correct the Content URI for 1.0.0 REST API; adde...
Posted by YolandaMDavis <gi...@git.apache.org>.
Github user YolandaMDavis commented on the issue:
https://github.com/apache/nifi/pull/752
@markap14 found an issue during testing. When attempting to run the SiteToSiteProvenanceReporterTask on a single node setup (attempting to report to a cluster) I did not have the site to site port set, so the task attempted to log a warning. In the process it received an IllegalAccessException when it was attempting to use the getLogger parent method to obtain a logger to log the warning. That causes setup to fail and the reporter to not execute (see attached log). I think changing the code in AbstractSiteToSiteReportingTask.java (starting at line 115) to use a locally declared logger variable defaulted using the getLogger() method will help (e.g. declaring final ComponentLog logger = getLogger(); and using logger in the EventReporter). Here is the current code with suggested changes in comments:
`
//suggest to add
//ComponentLog logger = getLogger();
final EventReporter eventReporter = new EventReporter() {
@Override
public void reportEvent(final Severity severity, final String category, final String message) {
switch (severity) {
case WARNING:
getLogger().warn(message); //suggest change to logger.warn(message);
break;
case ERROR:
getLogger().error(message); //suggest change to logger.error(message);
break;
default:
break;
}
}
};
`
When the SiteToSite port value was set on the cluster, communication occurred as expected and the contextURI in the captured json appears in the format expected. (e.g. http://localhost:808/nifi-api )
Attached are pictures of the error:
<img width="1440" alt="sitetositeloggererror" src="https://cloud.githubusercontent.com/assets/1371858/17322062/aaad713c-5869-11e6-8e1b-cd33e5b30b54.png">
---
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 #752: NIFI-2291: Correct the Content URI for 1.0.0 REST API; adde...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/752
@YolandaMDavis wow that's a good catch and is very... odd. I think this is actually a JDK bug. I'm about 99% sure that the anonymous inner class *should* be able to access that method. And if it can't, it should be a compile-time error not a runtime error. In fact, the Javadocs for that error even explicitly say:
> Normally, this error is caught by the compiler; this error can only occur at run time if the definition of a class has incompatibly changed.
I'm quite certain that the definition of the class has not changed. In any event, I believe the proposed solution is a reasonable one, so I implemented it and pushed a new commit. I also addressed your other feedback by making the URI path a constant. Good call.
Thanks for taking the time to review this. Let me know if you run into anything else. 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 #752: NIFI-2291: Correct the Content URI for 1.0.0 REST API; adde...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/752
Excellent, thanks, @YolandaMDavis ! Congratulations of your first merge :)
---
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 #752: NIFI-2291: Correct the Content URI for 1.0.0 REST AP...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/752
---
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 #752: NIFI-2291: Correct the Content URI for 1.0.0 REST AP...
Posted by YolandaMDavis <gi...@git.apache.org>.
Github user YolandaMDavis commented on a diff in the pull request:
https://github.com/apache/nifi/pull/752#discussion_r73095094
--- Diff: nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteProvenanceReportingTask.java ---
@@ -263,10 +271,15 @@ static JsonObject serialize(final JsonBuilderFactory factory, final JsonObjectBu
addField(builder, "actorHostname", hostname);
if (nifiUrl != null) {
- final String urlPrefix = nifiUrl.toString().replace(nifiUrl.getPath(), "");
- final String contentUriBase = urlPrefix + "/nifi-api/controller/provenance/events/" + event.getEventId() + "/content/";
- addField(builder, "contentURI", contentUriBase + "output");
- addField(builder, "previousContentURI", contentUriBase + "input");
+ // TO get URL Prefix, we just remove the /nifi from the end of the URL. We know that the URL ends with
+ // "/nifi" because the Property Validator enforces it
+ final String urlString = nifiUrl.toString();
+ final String urlPrefix = urlString.substring(0, urlString.length() - 5);
--- End diff --
Understood that validator enforces "/nifi" yet still seems to me like a place to use the URL methods to build URL or perhaps reference a static variable representing that string here (and in the Validator). Low likelihood for that path to change but if it happened this would at least be a quick fix or none required at all.
---
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 #752: NIFI-2291: Correct the Content URI for 1.0.0 REST API; adde...
Posted by YolandaMDavis <gi...@git.apache.org>.
Github user YolandaMDavis commented on the issue:
https://github.com/apache/nifi/pull/752
@markap14 all looks good, was able to test reporting task logging errors/warning without failing and reporting successfully with contentURI changes
+1
Will get this merged into master shortly (yay my first merge into Apache NiFi! :))
---
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.
---