You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/05/04 10:09:14 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1324: MINIFICPP-1749 Validate C2 property update values
lordgamez opened a new pull request, #1324:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324
Added verification of C2 property values while updating them, also changed the C2 response value in case no value could be updated due to validation failure or if the update is denied by the UpdatePolicyController to NOT_APPLIED instead of PARTIALLY_APPLIED. The PARTIALLY_APPLIED response is still sent if any of the requested updates could be finished.
https://issues.apache.org/jira/browse/MINIFICPP-1749
------------------------------------------
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:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-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 main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code 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?
- [ ] If applicable, have you updated the NOTICE file?
### 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 GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1324: MINIFICPP-1749 Validate C2 property update values
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1324:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324#discussion_r870434045
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -627,19 +628,21 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
changeUpdateState(update_property(entry.first, entry.second.to_string(), lifetime));
}
// apply changes and persist properties requested to be persisted
- if (result != state::UpdateState::NO_OPERATION && !configuration_->commitChanges()) {
+ auto propertyWasUpdated = [&](){ return result == state::UpdateState::FULLY_APPLIED || result == state::UpdateState::PARTIALLY_APPLIED; };
Review Comment:
The result could change in line 633
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1324: MINIFICPP-1749 Validate C2 property update values
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1324:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324#discussion_r871042006
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -627,19 +628,21 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
changeUpdateState(update_property(entry.first, entry.second.to_string(), lifetime));
}
// apply changes and persist properties requested to be persisted
- if (result != state::UpdateState::NO_OPERATION && !configuration_->commitChanges()) {
+ auto propertyWasUpdated = [&](){ return result == state::UpdateState::FULLY_APPLIED || result == state::UpdateState::PARTIALLY_APPLIED; };
Review Comment:
Updated in 8ee5795f8307b55ba1d0042379eacad695f9d26c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1324: MINIFICPP-1749 Validate C2 property update values
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1324:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324#discussion_r870359066
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -627,19 +628,21 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
changeUpdateState(update_property(entry.first, entry.second.to_string(), lifetime));
}
// apply changes and persist properties requested to be persisted
- if (result != state::UpdateState::NO_OPERATION && !configuration_->commitChanges()) {
+ auto propertyWasUpdated = [&](){ return result == state::UpdateState::FULLY_APPLIED || result == state::UpdateState::PARTIALLY_APPLIED; };
Review Comment:
wouldn't a bool variable suffice?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm closed pull request #1324: MINIFICPP-1749 Validate C2 property update values
Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #1324: MINIFICPP-1749 Validate C2 property update values
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1324: MINIFICPP-1749 Validate C2 property update values
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1324:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324#discussion_r870346764
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -627,19 +628,21 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
changeUpdateState(update_property(entry.first, entry.second.to_string(), lifetime));
}
// apply changes and persist properties requested to be persisted
- if (result != state::UpdateState::NO_OPERATION && !configuration_->commitChanges()) {
+ auto propertyWasUpdated = [&](){ return result == state::UpdateState::FULLY_APPLIED || result == state::UpdateState::PARTIALLY_APPLIED; };
Review Comment:
I only added this lambda for more readability as it is reused for checking if the property was updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1324: MINIFICPP-1749 Validate C2 property update values
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1324:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324#discussion_r871027729
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -627,19 +628,21 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
changeUpdateState(update_property(entry.first, entry.second.to_string(), lifetime));
}
// apply changes and persist properties requested to be persisted
- if (result != state::UpdateState::NO_OPERATION && !configuration_->commitChanges()) {
+ auto propertyWasUpdated = [&](){ return result == state::UpdateState::FULLY_APPLIED || result == state::UpdateState::PARTIALLY_APPLIED; };
Review Comment:
if `propertyWasUpdated()` returns `false` at first, then line 633 won't run and the result remains `false`, if it returns `true` at first and the commit fails, line 633 sets it to `PARTIALLY_APPLIED` which keeps its result `true`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1324: MINIFICPP-1749 Validate C2 property update values
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1324:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324#discussion_r870434045
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -627,19 +628,21 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
changeUpdateState(update_property(entry.first, entry.second.to_string(), lifetime));
}
// apply changes and persist properties requested to be persisted
- if (result != state::UpdateState::NO_OPERATION && !configuration_->commitChanges()) {
+ auto propertyWasUpdated = [&](){ return result == state::UpdateState::FULLY_APPLIED || result == state::UpdateState::PARTIALLY_APPLIED; };
Review Comment:
Unfortunately no, because the result could change in line 633
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1324: MINIFICPP-1749 Validate C2 property update values
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1324:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324#discussion_r870315733
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -627,19 +628,21 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
changeUpdateState(update_property(entry.first, entry.second.to_string(), lifetime));
}
// apply changes and persist properties requested to be persisted
- if (result != state::UpdateState::NO_OPERATION && !configuration_->commitChanges()) {
+ auto propertyWasUpdated = [&](){ return result == state::UpdateState::FULLY_APPLIED || result == state::UpdateState::PARTIALLY_APPLIED; };
Review Comment:
do we need the lambda, it seems that its return value is constant (even with the possible assignment in the first `if`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1324: MINIFICPP-1749 Validate C2 property update values
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1324:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1324#discussion_r871040486
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -627,19 +628,21 @@ void C2Agent::handlePropertyUpdate(const C2ContentResponse &resp) {
changeUpdateState(update_property(entry.first, entry.second.to_string(), lifetime));
}
// apply changes and persist properties requested to be persisted
- if (result != state::UpdateState::NO_OPERATION && !configuration_->commitChanges()) {
+ auto propertyWasUpdated = [&](){ return result == state::UpdateState::FULLY_APPLIED || result == state::UpdateState::PARTIALLY_APPLIED; };
Review Comment:
That's true, but I wasn't sure if changing it to a bool would be error-prone in the future if result calculation changes before we check for the need for a restart. But I suppose we can keep an eye on that and we have a test for the restart requests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org