You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by JPercivall <gi...@git.apache.org> on 2016/04/21 22:51:47 UTC
[GitHub] nifi-minifi pull request: MINIFI-17 Adding error handling of confi...
GitHub user JPercivall opened a pull request:
https://github.com/apache/nifi-minifi/pull/15
MINIFI-17 Adding error handling of configurations that fail to start …
…and a couple other small changes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/JPercivall/nifi-minifi MINIFI-17
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi/pull/15.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 #15
----
commit d3792ead7ca1207980e6388b93adf42e7e7a370c
Author: Joseph Percivall <jo...@yahoo.com>
Date: 2016-04-21T20:50:19Z
MINIFI-17 Adding error handling of configurations that fail to start and a couple other small changes
----
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60846573
--- Diff: minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/configuration/notifiers/FileChangeNotifier.java ---
@@ -63,15 +71,23 @@
}
@Override
- public void notifyListeners() {
+ public ListenerHandleResult[] notifyListeners() {
+ logger.info("Notifying Listeners of a change");
final File fileToRead = configFile.toFile();
+
+ ListenerHandleResult[] listenerHandleResults = new ListenerHandleResult[configurationChangeListeners.size()];
+ int count = 0;
for (final ConfigurationChangeListener listener : getChangeListeners()) {
try (final FileInputStream fis = new FileInputStream(fileToRead);) {
listener.handleChange(fis);
- } catch (IOException ex) {
- throw new IllegalStateException("Unable to read the changed file " + configFile, ex);
+ listenerHandleResults[count] = new ListenerHandleResult(listener);
+ } catch (IOException | ConfigurationChangeException ex) {
+ listenerHandleResults[count] = new ListenerHandleResult(listener, ex);
}
+ logger.info("Listener notification result:" + listenerHandleResults[count].toString());
+ count ++;
}
+ return new ListenerHandleResult[0];
--- End diff --
this should be listenerHandleResults. this is only returning a new empty array.
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-minifi/pull/15
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60741952
--- Diff: minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/RunMiNiFi.java ---
@@ -1379,7 +1430,7 @@ private void saveFile(final InputStream configInputStream, File configFile) {
}
}
- private void performTransformation(InputStream configIs, String configDestinationPath) {
+ public void performTransformation(InputStream configIs, String configDestinationPath) {
--- End diff --
That is true, I will make it package private
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60739792
--- Diff: minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/RunMiNiFi.java ---
@@ -1379,7 +1430,7 @@ private void saveFile(final InputStream configInputStream, File configFile) {
}
}
- private void performTransformation(InputStream configIs, String configDestinationPath) {
+ public void performTransformation(InputStream configIs, String configDestinationPath) {
--- End diff --
Understand the need for increased visibility but think protected or package private would be sufficient.
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60846453
--- Diff: minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/configuration/ConfigurationChangeNotifier.java ---
@@ -52,6 +52,5 @@
/**
* Provide the mechanism by which listeners are notified
*/
- void notifyListeners();
-
+ ListenerHandleResult[] notifyListeners();
--- End diff --
Would prefer to use a Collection opposed to an array
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60736740
--- Diff: minifi-assembly/pom.xml ---
@@ -261,7 +261,7 @@ limitations under the License.
<!-- nifi.properties: web properties -->
<nifi.web.war.directory>./lib</nifi.web.war.directory>
<nifi.web.http.host />
- <nifi.web.http.port>8080</nifi.web.http.port>
--- End diff --
Should be converted back to the typical 8080 default.
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60849783
--- Diff: minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/configuration/notifiers/RestChangeNotifier.java ---
@@ -227,15 +235,27 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
}
}
setConfigFile(configBuilder.substring(0,configBuilder.length()-1));
- notifyListeners();
- writeOutput(response, POST_TEXT, 200);
+ ListenerHandleResult[] listenerHandleResults = notifyListeners();
+
+ writeOutput(response, getPostText(listenerHandleResults), 200);
--- End diff --
Good call, I changed it to return the text status of the listeners but didn't change the status code. I can't tell whether the listener failed due to client or server error so I will just return 200 if all succeeded but 500 if any failed.
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60738272
--- Diff: minifi-assembly/pom.xml ---
@@ -261,7 +261,7 @@ limitations under the License.
<!-- nifi.properties: web properties -->
<nifi.web.war.directory>./lib</nifi.web.war.directory>
<nifi.web.http.host />
- <nifi.web.http.port>8080</nifi.web.http.port>
--- End diff --
I switched it because we're planning on getting rid of the UI anyway so currently it is primarily for debugging purposes and it's easier to debug when it doesn't have a conflicting default port as NiFi. If it makes more sense to switch it back I can then I would also change it in the ConfigTransformer (defaults to 8081 during transformation).
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60739656
--- Diff: minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/RunMiNiFi.java ---
@@ -1080,15 +1111,31 @@ public void start() throws IOException, InterruptedException {
final File reloadFile = getReloadFile(defaultLogger);
if (reloadFile.exists()) {
- defaultLogger.info("Currently reloading configuration. Will not restart MiNiFi.");
+ defaultLogger.info("Currently reloading configuration. Will wait to restart MiNiFi.");
Thread.sleep(5000L);
continue;
}
final boolean previouslyStarted = getNifiStarted();
if (!previouslyStarted) {
- defaultLogger.info("MiNiFi never started. Will not restart MiNiFi");
- return;
+ final File swapConfigFile = getSwapFile(defaultLogger);
+ if(swapConfigFile.exists()){
+ defaultLogger.info("Swap file exists, MiNiFi failed trying to change configuration. Reverting to old configuration.");
+
+ final String confDir = getBootstrapProperties().getProperty(CONF_DIR_KEY);
+ ((MiNiFiConfigurationChangeListener) changeListener).performTransformation(new FileInputStream(swapConfigFile), confDir);
--- End diff --
Would prefer to just declare the field of this class to just be a MiNiFiConfigurationChangeListener instead of having to do the cast.
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60846667
--- Diff: minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/configuration/notifiers/RestChangeNotifier.java ---
@@ -227,15 +235,27 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
}
}
setConfigFile(configBuilder.substring(0,configBuilder.length()-1));
- notifyListeners();
- writeOutput(response, POST_TEXT, 200);
+ ListenerHandleResult[] listenerHandleResults = notifyListeners();
+
+ writeOutput(response, getPostText(listenerHandleResults), 200);
--- End diff --
With the new collection of results, it seems like the returned status code should reflect the status of the attempted notifications.
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60741623
--- Diff: minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/RunMiNiFi.java ---
@@ -1080,15 +1111,31 @@ public void start() throws IOException, InterruptedException {
final File reloadFile = getReloadFile(defaultLogger);
if (reloadFile.exists()) {
- defaultLogger.info("Currently reloading configuration. Will not restart MiNiFi.");
+ defaultLogger.info("Currently reloading configuration. Will wait to restart MiNiFi.");
Thread.sleep(5000L);
continue;
}
final boolean previouslyStarted = getNifiStarted();
if (!previouslyStarted) {
- defaultLogger.info("MiNiFi never started. Will not restart MiNiFi");
- return;
+ final File swapConfigFile = getSwapFile(defaultLogger);
+ if(swapConfigFile.exists()){
+ defaultLogger.info("Swap file exists, MiNiFi failed trying to change configuration. Reverting to old configuration.");
+
+ final String confDir = getBootstrapProperties().getProperty(CONF_DIR_KEY);
+ ((MiNiFiConfigurationChangeListener) changeListener).performTransformation(new FileInputStream(swapConfigFile), confDir);
--- End diff --
I agree, changing
---
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-minifi pull request: MINIFI-17 Adding error handling of confi...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/15#discussion_r60849828
--- Diff: minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/configuration/ConfigurationChangeNotifier.java ---
@@ -52,6 +52,5 @@
/**
* Provide the mechanism by which listeners are notified
*/
- void notifyListeners();
-
+ ListenerHandleResult[] notifyListeners();
--- End diff --
Yup I agree
---
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.
---