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/11/28 15:25:19 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
lordgamez opened a new pull request, #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460
Added `nifi.flowfile.repository.directory.default` property with default value `${MINIFI_HOME}/flowfile_repository`
If repositories are set to empty value the default value is used instead of failure.
https://issues.apache.org/jira/browse/MINIFICPP-1995
-------------------
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] szaszm commented on a diff in pull request #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#discussion_r1035972201
##########
examples/kubernetes/daemon-set-log-collection/daemon-set-log-collection.yml:
##########
@@ -30,6 +30,7 @@ data:
nifi.provenance.repository.max.storage.time=1 MIN
nifi.provenance.repository.max.storage.size=1 MB
nifi.flowfile.repository.directory.default=${MINIFI_HOME}/flowfile_repository
+ nifi.flowfile.checkpoint.directory.default=${MINIFI_HOME}/flowfile_checkpoint
Review Comment:
I think the property is not relevant here, so I would leave it out from the example. I would ideally also remove all other properties that are using their default values, to keep the example minimal, but that doesn't need to happen in this PR.
##########
examples/kubernetes/sidecar-log-collection/sidecar-log-collection.yml:
##########
@@ -32,6 +32,7 @@ data:
nifi.provenance.repository.max.storage.time=1 MIN
nifi.provenance.repository.max.storage.size=1 MB
nifi.flowfile.repository.directory.default=${MINIFI_HOME}/flowfile_repository
+ nifi.flowfile.checkpoint.directory.default=${MINIFI_HOME}/flowfile_checkpoint
Review Comment:
I think the property is not relevant here, so I would leave it out from the example. I would ideally also remove all other properties that are using their default values, to keep the example minimal, but that doesn't need to happen in this PR.
##########
CONFIGURE.md:
##########
@@ -119,23 +119,23 @@ for TCP and secure HTTPS communications.
name: tominifi
max concurrent tasks: 2
Properties:
- SSL Context Service: SSLServiceName
Review Comment:
The old indentation was the correct one for SSL Context Service, since it's a property.
--
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 #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#discussion_r1059904518
##########
extensions/rocksdb-repos/FlowFileRepository.h:
##########
@@ -100,11 +100,17 @@ class FlowFileRepository : public ThreadedRepository, public SwapManager {
config_ = configure;
std::string value;
- if (configure->get(Configure::nifi_flowfile_repository_directory_default, value)) {
+ if (configure->get(Configure::nifi_flowfile_repository_directory_default, value) && !value.empty()) {
directory_ = value;
}
logger_->log_debug("NiFi FlowFile Repository Directory %s", directory_);
+ value.clear();
+ if (configure->get(Configure::nifi_flowfile_checkpoint_directory_default, value) && !value.empty()) {
+ checkpoint_dir_ = value;
+ }
+ logger_->log_debug("NiFi FlowFile Checkpoint Directory %s", checkpoint_dir_);
Review Comment:
Added fix in 9b7fd9553203ec6bfae427546e0fa214e695f17f
--
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] fgerlits commented on pull request #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
fgerlits commented on PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#issuecomment-1377200086
Unfortunately, the code did not compile on Windows after 373dbdd98b16d1a45788c5bb10c926368af0c891, so I have reverted both c07ee3e10c54a19820f7b5ac8d2ee63768ed5759 and 373dbdd98b16d1a45788c5bb10c926368af0c891, and added a `.string()` to the log line: 378501d61d0d0a98c1688f28c7a35161af5e0d3a
--
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 #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#discussion_r1063442029
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -72,6 +76,8 @@ inline decltype(auto) conditional_convert(const Arg& val) {
return val;
} else if constexpr (std::is_same_v<decltype(std::declval<const Arg&>().c_str()), const char*>) {
return val.c_str();
+ } else if constexpr (std::is_same_v<decltype(std::declval<const Arg&>().string()), std::string>) {
+ return val.string().c_str();
Review Comment:
the `::string() const` method could return a temporary which is destroyed before using the result of `c_str()` leaving us with UB, I think for this to be safe, the `conditional_stringify` functions should call the `::string() const` method, the returned temporary string is kept alive until the end of the full expression the `conditional_stringify` is part of, which is after `format_string` returns
--
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 pull request #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
lordgamez commented on PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#issuecomment-1377203501
> Unfortunately, the code did not compile on Windows after [373dbdd](https://github.com/apache/nifi-minifi-cpp/commit/373dbdd98b16d1a45788c5bb10c926368af0c891), so I have reverted both [c07ee3e](https://github.com/apache/nifi-minifi-cpp/commit/c07ee3e10c54a19820f7b5ac8d2ee63768ed5759) and [373dbdd](https://github.com/apache/nifi-minifi-cpp/commit/373dbdd98b16d1a45788c5bb10c926368af0c891), and added a `.string()` to the log line: [378501d](https://github.com/apache/nifi-minifi-cpp/commit/378501d61d0d0a98c1688f28c7a35161af5e0d3a)
Thanks for the update, you are right it's better to keep it simple at this point.
--
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 #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#discussion_r1063609446
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -72,6 +76,8 @@ inline decltype(auto) conditional_convert(const Arg& val) {
return val;
} else if constexpr (std::is_same_v<decltype(std::declval<const Arg&>().c_str()), const char*>) {
return val.c_str();
+ } else if constexpr (std::is_same_v<decltype(std::declval<const Arg&>().string()), std::string>) {
+ return val.string().c_str();
Review Comment:
Good catch, updated in fb658f6abd33164157570344436dddae13700823
--
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 #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#discussion_r1036007632
##########
CONFIGURE.md:
##########
@@ -119,23 +119,23 @@ for TCP and secure HTTPS communications.
name: tominifi
max concurrent tasks: 2
Properties:
- SSL Context Service: SSLServiceName
Review Comment:
Good catch, updated in cbac8731aa8f068600e1cb76a7fc0b4bdc6e72fc
##########
examples/kubernetes/daemon-set-log-collection/daemon-set-log-collection.yml:
##########
@@ -30,6 +30,7 @@ data:
nifi.provenance.repository.max.storage.time=1 MIN
nifi.provenance.repository.max.storage.size=1 MB
nifi.flowfile.repository.directory.default=${MINIFI_HOME}/flowfile_repository
+ nifi.flowfile.checkpoint.directory.default=${MINIFI_HOME}/flowfile_checkpoint
Review Comment:
Added jira ticket for this impovement: https://issues.apache.org/jira/browse/MINIFICPP-2004
##########
examples/kubernetes/sidecar-log-collection/sidecar-log-collection.yml:
##########
@@ -32,6 +32,7 @@ data:
nifi.provenance.repository.max.storage.time=1 MIN
nifi.provenance.repository.max.storage.size=1 MB
nifi.flowfile.repository.directory.default=${MINIFI_HOME}/flowfile_repository
+ nifi.flowfile.checkpoint.directory.default=${MINIFI_HOME}/flowfile_checkpoint
Review Comment:
Added jira ticket for this impovement: https://issues.apache.org/jira/browse/MINIFICPP-2004
--
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 #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#discussion_r1034465236
##########
encrypt-config/tests/resources/minifi.properties:
##########
@@ -24,6 +24,7 @@ nifi.provenance.repository.directory.default=${MINIFI_HOME}/provenance_repositor
nifi.provenance.repository.max.storage.time=1 MIN
nifi.provenance.repository.max.storage.size=1 MB
nifi.flowfile.repository.directory.default=${MINIFI_HOME}/flowfile_repository
+nifi.flowfile.checkpoint.directory.default=${MINIFI_HOME}/flowfile_checkpoint
Review Comment:
Updated in 28dfa20abfc159c5da28c85026fae4f459b430bb
--
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 commented on a diff in pull request #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#discussion_r1034444422
##########
encrypt-config/tests/resources/minifi.properties:
##########
@@ -24,6 +24,7 @@ nifi.provenance.repository.directory.default=${MINIFI_HOME}/provenance_repositor
nifi.provenance.repository.max.storage.time=1 MIN
nifi.provenance.repository.max.storage.size=1 MB
nifi.flowfile.repository.directory.default=${MINIFI_HOME}/flowfile_repository
+nifi.flowfile.checkpoint.directory.default=${MINIFI_HOME}/flowfile_checkpoint
Review Comment:
I think we can omit this from test resources if it's the same as the default. It makes sense to add to the example config, but in test resources, it's just redundant.
--
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] fgerlits commented on a diff in pull request #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
fgerlits commented on code in PR #1460:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460#discussion_r1056437169
##########
extensions/rocksdb-repos/FlowFileRepository.h:
##########
@@ -100,11 +100,17 @@ class FlowFileRepository : public ThreadedRepository, public SwapManager {
config_ = configure;
std::string value;
- if (configure->get(Configure::nifi_flowfile_repository_directory_default, value)) {
+ if (configure->get(Configure::nifi_flowfile_repository_directory_default, value) && !value.empty()) {
directory_ = value;
}
logger_->log_debug("NiFi FlowFile Repository Directory %s", directory_);
+ value.clear();
+ if (configure->get(Configure::nifi_flowfile_checkpoint_directory_default, value) && !value.empty()) {
+ checkpoint_dir_ = value;
+ }
+ logger_->log_debug("NiFi FlowFile Checkpoint Directory %s", checkpoint_dir_);
Review Comment:
I think you need to either add `.string()` here, or make the conversion templates in `Logger.h` smarter to accept `path`s.
--
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] fgerlits closed pull request #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
Posted by GitBox <gi...@apache.org>.
fgerlits closed pull request #1460: MINIFICPP-1995 Add configuring path for flowfile_checkpoint directory
URL: https://github.com/apache/nifi-minifi-cpp/pull/1460
--
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