You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by achristianson <gi...@git.apache.org> on 2017/10/30 16:59:57 UTC
[GitHub] nifi-minifi-cpp pull request #162: MINIFICPP-273 Simplify configuration of M...
GitHub user achristianson opened a pull request:
https://github.com/apache/nifi-minifi-cpp/pull/162
MINIFICPP-273 Simplify configuration of MiNiFi - C++ by allowing defa…
…ult values for scheduling strategy and period.
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
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 MINIFI-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:
- [x] 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)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [x] 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/achristianson/nifi-minifi-cpp MINIFICPP-273
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi-cpp/pull/162.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 #162
----
commit a4f27dd1b6e15c927971741656f2c037529ddde8
Author: Andy I. Christianson <an...@andyic.org>
Date: 2017-10-25T20:35:04Z
MINIFICPP-273 Simplify configuration of MiNiFi - C++ by allowing default values for scheduling strategy and period.
----
---
[GitHub] nifi-minifi-cpp pull request #162: MINIFICPP-273 Simplify configuration of M...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/162#discussion_r148019697
--- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
@@ -698,6 +703,34 @@ void YamlConfiguration::checkRequiredField(YAML::Node *yamlNode, const std::stri
}
}
+YAML::Node YamlConfiguration::getOptionalField(YAML::Node *yamlNode,
+ const std::string &fieldName,
+ const YAML::Node &defaultValue,
+ const std::string &yamlSection,
+ const std::string &providedInfoMessage) {
+ std::string infoMessage = providedInfoMessage;
+ auto result = yamlNode->as<YAML::Node>()[fieldName];
+ if (!result) {
+ if (infoMessage.empty()) {
+ // Build a helpful info message for the user to inform them that a default is being used
+ infoMessage =
+ yamlNode->as<YAML::Node>()["name"] ?
+ "Using default value for optional field '" + fieldName + "' in component named '"
+ + yamlNode->as<YAML::Node>()["name"].as<std::string>() + "'" :
+ "Using default value for optional field '" + fieldName + "' ";
+ if (!yamlSection.empty()) {
+ infoMessage += " [in '" + yamlSection + "' section of configuration file]: ";
+ }
+
+ infoMessage += defaultValue.as<std::string>();
+ }
+ logger_->log_info(infoMessage.c_str());
+ result = defaultValue;
--- End diff --
My bad. Will fix & re-push.
I didn't notice any issues in make linter, though.
---
[GitHub] nifi-minifi-cpp pull request #162: MINIFICPP-273 Simplify configuration of M...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/162#discussion_r148027976
--- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
@@ -118,14 +119,18 @@ void YamlConfiguration::parseProcessorNodeYaml(YAML::Node processorsNode, core::
}
processor->setName(procCfg.name);
- checkRequiredField(&procNode, "scheduling strategy",
- CONFIG_YAML_PROCESSORS_KEY);
- procCfg.schedulingStrategy = procNode["scheduling strategy"].as<std::string>();
+ auto strategyNode = getOptionalField(&procNode,
+ "scheduling strategy",
+ YAML::Node("EVENT_DRIVEN"),
--- End diff --
Move the Yaml::Node(...), or the getOptionalField call? And where to?
---
[GitHub] nifi-minifi-cpp pull request #162: MINIFICPP-273 Simplify configuration of M...
Posted by minifirocks <gi...@git.apache.org>.
Github user minifirocks commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/162#discussion_r147889510
--- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
@@ -698,6 +703,34 @@ void YamlConfiguration::checkRequiredField(YAML::Node *yamlNode, const std::stri
}
}
+YAML::Node YamlConfiguration::getOptionalField(YAML::Node *yamlNode,
+ const std::string &fieldName,
+ const YAML::Node &defaultValue,
+ const std::string &yamlSection,
+ const std::string &providedInfoMessage) {
+ std::string infoMessage = providedInfoMessage;
+ auto result = yamlNode->as<YAML::Node>()[fieldName];
+ if (!result) {
+ if (infoMessage.empty()) {
+ // Build a helpful info message for the user to inform them that a default is being used
+ infoMessage =
+ yamlNode->as<YAML::Node>()["name"] ?
+ "Using default value for optional field '" + fieldName + "' in component named '"
+ + yamlNode->as<YAML::Node>()["name"].as<std::string>() + "'" :
+ "Using default value for optional field '" + fieldName + "' ";
+ if (!yamlSection.empty()) {
+ infoMessage += " [in '" + yamlSection + "' section of configuration file]: ";
+ }
+
+ infoMessage += defaultValue.as<std::string>();
+ }
+ logger_->log_info(infoMessage.c_str());
+ result = defaultValue;
--- End diff --
no return of the result. please run make linter
---
[GitHub] nifi-minifi-cpp pull request #162: MINIFICPP-273 Simplify configuration of M...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/162#discussion_r148025853
--- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
@@ -118,14 +119,18 @@ void YamlConfiguration::parseProcessorNodeYaml(YAML::Node processorsNode, core::
}
processor->setName(procCfg.name);
- checkRequiredField(&procNode, "scheduling strategy",
- CONFIG_YAML_PROCESSORS_KEY);
- procCfg.schedulingStrategy = procNode["scheduling strategy"].as<std::string>();
+ auto strategyNode = getOptionalField(&procNode,
+ "scheduling strategy",
+ YAML::Node("EVENT_DRIVEN"),
--- End diff --
We should move this elsewhere.
---
[GitHub] nifi-minifi-cpp issue #162: MINIFICPP-273 Simplify configuration of MiNiFi -...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/162
Ready for another look.
---
[GitHub] nifi-minifi-cpp issue #162: MINIFICPP-273 Simplify configuration of MiNiFi -...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/162
Fixed review issues. Ready for more review/merge.
---
[GitHub] nifi-minifi-cpp pull request #162: MINIFICPP-273 Simplify configuration of M...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/162#discussion_r148037953
--- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
@@ -118,14 +119,18 @@ void YamlConfiguration::parseProcessorNodeYaml(YAML::Node processorsNode, core::
}
processor->setName(procCfg.name);
- checkRequiredField(&procNode, "scheduling strategy",
- CONFIG_YAML_PROCESSORS_KEY);
- procCfg.schedulingStrategy = procNode["scheduling strategy"].as<std::string>();
+ auto strategyNode = getOptionalField(&procNode,
+ "scheduling strategy",
+ YAML::Node("EVENT_DRIVEN"),
--- End diff --
Sorry I meant the YAML::Node. I shouldn't have been so terse.
Here is a better way of putting it: Could we make it so that we centralized the defaults in one place so in case of change it's easier to track down? I know this isn't really the guise of the PR; however, it may improve the hygiene of this class to do so or make a follow on ticket.
---
[GitHub] nifi-minifi-cpp pull request #162: MINIFICPP-273 Simplify configuration of M...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-minifi-cpp/pull/162
---