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/06 09:19:01 UTC
[GitHub] [nifi-minifi-cpp] adam-markovics opened a new pull request, #1328: MINIFICPP-1812 - Clean up Repository threads
adam-markovics opened a new pull request, #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328
https://issues.apache.org/jira/browse/MINIFICPP-1812
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r959228721
##########
libminifi/include/core/repository/FileSystemRepository.h:
##########
@@ -25,49 +25,39 @@
#include "../ContentRepository.h"
#include "properties/Configure.h"
#include "core/logging/LoggerFactory.h"
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace core {
-namespace repository {
+
+namespace org::apache::nifi::minifi::core::repository {
/**
* FileSystemRepository is a content repository that stores data onto the local file system.
*/
class FileSystemRepository : public core::ContentRepository, public core::CoreComponent {
public:
FileSystemRepository(std::string name = getClassName<FileSystemRepository>()) // NOLINT
- : core::CoreComponent(name),
- logger_(logging::LoggerFactory<FileSystemRepository>::getLogger()) {
+ : core::CoreComponent(name),
+ logger_(logging::LoggerFactory<FileSystemRepository>::getLogger()) {
Review Comment:
Done.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r923434200
##########
libminifi/include/core/Repository.h:
##########
@@ -20,89 +20,77 @@
#ifndef LIBMINIFI_INCLUDE_CORE_REPOSITORY_H_
#define LIBMINIFI_INCLUDE_CORE_REPOSITORY_H_
-#include <memory>
-#include <utility>
#include <atomic>
#include <cstdint>
#include <cstring>
-#include <iostream>
#include <map>
+#include <memory>
#include <set>
#include <string>
-#include <thread>
+#include <utility>
#include <vector>
+
+#include "Core.h"
+#include "ResourceClaim.h"
+#include "core/Connectable.h"
#include "core/ContentRepository.h"
+#include "core/Property.h"
#include "core/SerializableComponent.h"
-#include "properties/Configure.h"
#include "core/logging/LoggerConfiguration.h"
-#include "core/Property.h"
-#include "ResourceClaim.h"
-#include "utils/TimeUtil.h"
-#include "utils/StringUtils.h"
-#include "Core.h"
-#include "core/Connectable.h"
-#include "core/TraceableResource.h"
+#include "properties/Configure.h"
#include "utils/BackTrace.h"
+#include "utils/Literals.h"
+#include "utils/StringUtils.h"
+#include "utils/TimeUtil.h"
#ifndef WIN32
#include <sys/stat.h>
#endif
namespace org::apache::nifi::minifi::core {
-#define REPOSITORY_DIRECTORY "./repo"
-#define MAX_REPOSITORY_STORAGE_SIZE (10*1024*1024) // 10M
+constexpr auto REPOSITORY_DIRECTORY = "./repo";
+constexpr auto MAX_REPOSITORY_STORAGE_SIZE = 10_MiB;
constexpr auto MAX_REPOSITORY_ENTRY_LIFE_TIME = std::chrono::minutes(10);
constexpr auto REPOSITORY_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class Repository : public virtual core::SerializableComponent, public core::TraceableResource {
+class Repository : public virtual core::SerializableComponent {
Review Comment:
I chose `initialize`
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r918878788
##########
libminifi/include/core/ThreadedRepository.h:
##########
@@ -0,0 +1,86 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <memory>
+#include <thread>
+
+#include "Repository.h"
+#include "TraceableResource.h"
+
+namespace org::apache::nifi::minifi::core {
+
+class ThreadedRepository : public core::Repository, public core::TraceableResource {
+ public:
+ using Repository::Repository;
+
+ ~ThreadedRepository() override {
+ if (running_.load()) {
+ logger_->log_error("Thread of %s should have been stopped in subclass before ThreadedRepository's destruction", name_);
+ }
+ }
+
+ // Starts repository monitor thread
+ void start() {
+ if (running_.exchange(true)) {
+ return;
+ }
+ if (purge_period_ <= std::chrono::milliseconds(0)) {
+ return;
+ }
+ getThread() = std::thread(&ThreadedRepository::run, this);
Review Comment:
I don't think there is such a restriction, `&ThreadedRepository::run` is a pointer to member function which, together with the `this`, seem to be a suitable arguments for the `std::thread` constructor, the behind the scenes magic (most likely offset into the vtable) makes sure that when called the overridden method based on the object's dynamic type is called
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #1328: MINIFICPP-1812 - Clean up Repository threads
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r923433814
##########
libminifi/include/core/RepositoryFactory.h:
##########
@@ -49,10 +45,14 @@ std::unique_ptr<core::Repository> createRepository(const std::string& configurat
*/
std::unique_ptr<core::ContentRepository> createContentRepository(const std::string& configuration_class_name, bool fail_safe = false, const std::string& repo_name = "");
-} // namespace core
-} // namespace minifi
-} // namespace nifi
-} // namespace apache
-} // namespace org
+/**
+ * Create a threaded repository represented by the configuration class name
+ * @param configuration_class_name configuration class name
+ * @param fail_safe determines whether or not to make the default class if configuration_class_name is invalid
+ * @param repo_name name of the repository
+ */
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, const std::string& repo_name = "");
Review Comment:
`core::createRepository` has been deleted
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r936542023
##########
libminifi/include/core/ThreadedRepository.h:
##########
@@ -0,0 +1,106 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <memory>
+#include <thread>
+
+#include "Repository.h"
+#include "TraceableResource.h"
+
+namespace org::apache::nifi::minifi::core {
+
+class ThreadedRepository : public core::Repository, public core::TraceableResource {
+ public:
+ using Repository::Repository;
+
+ ~ThreadedRepository() override {
+ if (running_state_.load() != RUNNING_STATE::STOPPED) {
+ logger_->log_error("Thread of %s should have been stopped in subclass before ThreadedRepository's destruction", name_);
+ }
+ }
+
+ bool initialize(const std::shared_ptr<Configure>& /*configure*/) override {
+ return true;
+ }
+
+ // Starts repository monitor thread
+ bool start() override {
+ // if STOPPED, turn to STARTING, otherwise return
+ RUNNING_STATE expected{RUNNING_STATE::STOPPED};
Review Comment:
Done.
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r918878788
##########
libminifi/include/core/ThreadedRepository.h:
##########
@@ -0,0 +1,86 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <memory>
+#include <thread>
+
+#include "Repository.h"
+#include "TraceableResource.h"
+
+namespace org::apache::nifi::minifi::core {
+
+class ThreadedRepository : public core::Repository, public core::TraceableResource {
+ public:
+ using Repository::Repository;
+
+ ~ThreadedRepository() override {
+ if (running_.load()) {
+ logger_->log_error("Thread of %s should have been stopped in subclass before ThreadedRepository's destruction", name_);
+ }
+ }
+
+ // Starts repository monitor thread
+ void start() {
+ if (running_.exchange(true)) {
+ return;
+ }
+ if (purge_period_ <= std::chrono::milliseconds(0)) {
+ return;
+ }
+ getThread() = std::thread(&ThreadedRepository::run, this);
Review Comment:
I don't think there is such a restriction, `&ThreadedRepository::run` is a pointer to member function which, together with the `this`, seem to be a suitable arguments for the `std::thread` constructor, the behind the scenes magic (most likely offset into the vtable) makes sure that when called the overridden method based on the objects dynamic type is called
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r924439624
##########
libminifi/include/core/Repository.h:
##########
@@ -20,89 +20,77 @@
#ifndef LIBMINIFI_INCLUDE_CORE_REPOSITORY_H_
#define LIBMINIFI_INCLUDE_CORE_REPOSITORY_H_
-#include <memory>
-#include <utility>
#include <atomic>
#include <cstdint>
#include <cstring>
-#include <iostream>
#include <map>
+#include <memory>
#include <set>
#include <string>
-#include <thread>
+#include <utility>
#include <vector>
+
+#include "Core.h"
+#include "ResourceClaim.h"
+#include "core/Connectable.h"
#include "core/ContentRepository.h"
+#include "core/Property.h"
#include "core/SerializableComponent.h"
-#include "properties/Configure.h"
#include "core/logging/LoggerConfiguration.h"
-#include "core/Property.h"
-#include "ResourceClaim.h"
-#include "utils/TimeUtil.h"
-#include "utils/StringUtils.h"
-#include "Core.h"
-#include "core/Connectable.h"
-#include "core/TraceableResource.h"
+#include "properties/Configure.h"
#include "utils/BackTrace.h"
+#include "utils/Literals.h"
+#include "utils/StringUtils.h"
+#include "utils/TimeUtil.h"
#ifndef WIN32
#include <sys/stat.h>
#endif
namespace org::apache::nifi::minifi::core {
-#define REPOSITORY_DIRECTORY "./repo"
-#define MAX_REPOSITORY_STORAGE_SIZE (10*1024*1024) // 10M
+constexpr auto REPOSITORY_DIRECTORY = "./repo";
+constexpr auto MAX_REPOSITORY_STORAGE_SIZE = 10_MiB;
constexpr auto MAX_REPOSITORY_ENTRY_LIFE_TIME = std::chrono::minutes(10);
constexpr auto REPOSITORY_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class Repository : public virtual core::SerializableComponent, public core::TraceableResource {
+class Repository : public virtual core::SerializableComponent {
Review Comment:
`ThreadedRepository` is gone, in `Repository` `run` and `getThread` are already pure virtual.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r928936020
##########
libminifi/include/core/repository/VolatileContentRepository.h:
##########
@@ -68,55 +59,55 @@ class VolatileContentRepository :
}
master_list_.clear();
}
+ stop();
}
/**
* Initialize the volatile content repo
* @param configure configuration
*/
- virtual bool initialize(const std::shared_ptr<Configure> &configure);
-
- /**
- * Stop any thread associated with the volatile content repository.
- */
- virtual void stop();
+ bool initialize(const std::shared_ptr<Configure> &configure) override;
/**
* Creates writable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream the consumer will write to.
*/
- virtual std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append);
+ std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append) override;
/**
* Creates readable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream from which the consumer will read..
*/
- virtual std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim);
+ std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim) override;
- virtual bool exists(const minifi::ResourceClaim &streamId);
+ bool exists(const minifi::ResourceClaim &streamId) override;
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool close(const minifi::ResourceClaim &claim) {
+ bool close(const minifi::ResourceClaim &claim) override {
return remove(claim);
}
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool remove(const minifi::ResourceClaim &claim);
+ bool remove(const minifi::ResourceClaim &claim) override;
+
+ private:
+ void run() override {
+ }
- protected:
- virtual void start();
+ std::thread& getThread() override {
+ return thread_;
+ }
Review Comment:
The previous one was like that. You asked to be like this: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r923437527
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r918759741
##########
libminifi/include/core/RepositoryFactory.h:
##########
@@ -49,10 +45,14 @@ std::unique_ptr<core::Repository> createRepository(const std::string& configurat
*/
std::unique_ptr<core::ContentRepository> createContentRepository(const std::string& configuration_class_name, bool fail_safe = false, const std::string& repo_name = "");
-} // namespace core
-} // namespace minifi
-} // namespace nifi
-} // namespace apache
-} // namespace org
+/**
+ * Create a threaded repository represented by the configuration class name
+ * @param configuration_class_name configuration class name
+ * @param fail_safe determines whether or not to make the default class if configuration_class_name is invalid
+ * @param repo_name name of the repository
+ */
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, const std::string& repo_name = "");
Review Comment:
Why keep `createRepository` if everything uses ThreadedRepositories?
##########
libminifi/include/core/ThreadedRepository.h:
##########
@@ -0,0 +1,86 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <memory>
+#include <thread>
+
+#include "Repository.h"
+#include "TraceableResource.h"
+
+namespace org::apache::nifi::minifi::core {
+
+class ThreadedRepository : public core::Repository, public core::TraceableResource {
+ public:
+ using Repository::Repository;
+
+ ~ThreadedRepository() override {
+ if (running_.load()) {
+ logger_->log_error("Thread of %s should have been stopped in subclass before ThreadedRepository's destruction", name_);
+ }
+ }
+
+ // Starts repository monitor thread
+ void start() {
+ if (running_.exchange(true)) {
+ return;
+ }
+ if (purge_period_ <= std::chrono::milliseconds(0)) {
+ return;
+ }
+ getThread() = std::thread(&ThreadedRepository::run, this);
Review Comment:
I think you can't take the address of a virtual member function and expect the derived class implementation to be called. If it works, it's probably a compiler extension, not standard. Use a lambda instead.
##########
libminifi/include/core/Repository.h:
##########
@@ -20,89 +20,77 @@
#ifndef LIBMINIFI_INCLUDE_CORE_REPOSITORY_H_
#define LIBMINIFI_INCLUDE_CORE_REPOSITORY_H_
-#include <memory>
-#include <utility>
#include <atomic>
#include <cstdint>
#include <cstring>
-#include <iostream>
#include <map>
+#include <memory>
#include <set>
#include <string>
-#include <thread>
+#include <utility>
#include <vector>
+
+#include "Core.h"
+#include "ResourceClaim.h"
+#include "core/Connectable.h"
#include "core/ContentRepository.h"
+#include "core/Property.h"
#include "core/SerializableComponent.h"
-#include "properties/Configure.h"
#include "core/logging/LoggerConfiguration.h"
-#include "core/Property.h"
-#include "ResourceClaim.h"
-#include "utils/TimeUtil.h"
-#include "utils/StringUtils.h"
-#include "Core.h"
-#include "core/Connectable.h"
-#include "core/TraceableResource.h"
+#include "properties/Configure.h"
#include "utils/BackTrace.h"
+#include "utils/Literals.h"
+#include "utils/StringUtils.h"
+#include "utils/TimeUtil.h"
#ifndef WIN32
#include <sys/stat.h>
#endif
namespace org::apache::nifi::minifi::core {
-#define REPOSITORY_DIRECTORY "./repo"
-#define MAX_REPOSITORY_STORAGE_SIZE (10*1024*1024) // 10M
+constexpr auto REPOSITORY_DIRECTORY = "./repo";
+constexpr auto MAX_REPOSITORY_STORAGE_SIZE = 10_MiB;
constexpr auto MAX_REPOSITORY_ENTRY_LIFE_TIME = std::chrono::minutes(10);
constexpr auto REPOSITORY_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class Repository : public virtual core::SerializableComponent, public core::TraceableResource {
+class Repository : public virtual core::SerializableComponent {
Review Comment:
Does it make sense to instantiate a Repository? If not, I would make it abstract. There are plenty of good candidate member functions, like `isNoop`, `flush`, `initialize`, `Put`, `MultiPut`, `Delete`, etc.
##########
controller/Controller.h:
##########
@@ -246,15 +247,24 @@ std::shared_ptr<org::apache::nifi::minifi::core::controller::ControllerService>
configuration->get(org::apache::nifi::minifi::Configure::nifi_provenance_repository_class_name, prov_repo_class);
// Create repos for flow record and provenance
- const std::shared_ptr prov_repo = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const std::shared_ptr prov_repo_base = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const auto prov_repo = std::dynamic_pointer_cast<org::apache::nifi::minifi::core::ThreadedRepository>(prov_repo_base);
Review Comment:
Why do you use normal `core::createRepository` here, while `core::createThreadedRepository` in main?
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r929052167
##########
libminifi/include/core/repository/VolatileContentRepository.h:
##########
@@ -68,55 +59,55 @@ class VolatileContentRepository :
}
master_list_.clear();
}
+ stop();
}
/**
* Initialize the volatile content repo
* @param configure configuration
*/
- virtual bool initialize(const std::shared_ptr<Configure> &configure);
-
- /**
- * Stop any thread associated with the volatile content repository.
- */
- virtual void stop();
+ bool initialize(const std::shared_ptr<Configure> &configure) override;
/**
* Creates writable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream the consumer will write to.
*/
- virtual std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append);
+ std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append) override;
/**
* Creates readable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream from which the consumer will read..
*/
- virtual std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim);
+ std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim) override;
- virtual bool exists(const minifi::ResourceClaim &streamId);
+ bool exists(const minifi::ResourceClaim &streamId) override;
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool close(const minifi::ResourceClaim &claim) {
+ bool close(const minifi::ResourceClaim &claim) override {
return remove(claim);
}
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool remove(const minifi::ResourceClaim &claim);
+ bool remove(const minifi::ResourceClaim &claim) override;
+
+ private:
+ void run() override {
+ }
- protected:
- virtual void start();
+ std::thread& getThread() override {
+ return thread_;
+ }
Review Comment:
You're right, I asked for it, because I thought that all repositories are "threaded". Obviously that's not the case. Additionally, I don't think `ThreadedRepository`, or the "threadedness" of a repository is something user code should be concerned with, so I don't say we should go back to the previous revision. But an empty/noop implementation is basically a violation of the [LSP](https://en.wikipedia.org/wiki/Liskov_substitution_principle).
My first thought would be to add a thread to every "threaded" repostitory, and implement the start/stop logic in all such leaf classes. That would go against [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself). That could be solved by reintroducing `ThreadedRepository` as an implementation helper, **not exposed to user code**. This is still an anti-pattern, by reusing implementation through inheritance, but this seems to be the least evil, and if we can enforce no naming of this class outside of Repository implementations, then it's relatively contained and harmless IMO. I would do this. If you know better, please comment it, so we can discuss.
In general, you understand your code best. When I comment an idea, and you run into an issue with it (e.g. [LSP](https://en.wikipedia.org/wiki/Liskov_substitution_principle) violation in the last case), or I just comment something stupid, please ask or push back. Solving a small problem by making a bigger one is not good.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r931151104
##########
libminifi/include/core/repository/VolatileContentRepository.h:
##########
@@ -68,55 +59,55 @@ class VolatileContentRepository :
}
master_list_.clear();
}
+ stop();
}
/**
* Initialize the volatile content repo
* @param configure configuration
*/
- virtual bool initialize(const std::shared_ptr<Configure> &configure);
-
- /**
- * Stop any thread associated with the volatile content repository.
- */
- virtual void stop();
+ bool initialize(const std::shared_ptr<Configure> &configure) override;
/**
* Creates writable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream the consumer will write to.
*/
- virtual std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append);
+ std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append) override;
/**
* Creates readable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream from which the consumer will read..
*/
- virtual std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim);
+ std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim) override;
- virtual bool exists(const minifi::ResourceClaim &streamId);
+ bool exists(const minifi::ResourceClaim &streamId) override;
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool close(const minifi::ResourceClaim &claim) {
+ bool close(const minifi::ResourceClaim &claim) override {
return remove(claim);
}
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool remove(const minifi::ResourceClaim &claim);
+ bool remove(const minifi::ResourceClaim &claim) override;
+
+ private:
+ void run() override {
+ }
- protected:
- virtual void start();
+ std::thread& getThread() override {
+ return thread_;
+ }
Review Comment:
It looks like the the threadedness of a repository must be exposed, because for example [FlowController uses it](https://github.com/adam-markovics/nifi-minifi-cpp/blob/53fbb264cb00fbb4189a2fb1ae7978c6913d3a1c/libminifi/src/FlowController.cpp#L381)
A threadless repo should not have a `start` method, it would be worse in my opinion.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r934659084
##########
libminifi/include/core/repository/VolatileContentRepository.h:
##########
@@ -68,55 +59,55 @@ class VolatileContentRepository :
}
master_list_.clear();
}
+ stop();
}
/**
* Initialize the volatile content repo
* @param configure configuration
*/
- virtual bool initialize(const std::shared_ptr<Configure> &configure);
-
- /**
- * Stop any thread associated with the volatile content repository.
- */
- virtual void stop();
+ bool initialize(const std::shared_ptr<Configure> &configure) override;
/**
* Creates writable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream the consumer will write to.
*/
- virtual std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append);
+ std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append) override;
/**
* Creates readable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream from which the consumer will read..
*/
- virtual std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim);
+ std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim) override;
- virtual bool exists(const minifi::ResourceClaim &streamId);
+ bool exists(const minifi::ResourceClaim &streamId) override;
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool close(const minifi::ResourceClaim &claim) {
+ bool close(const minifi::ResourceClaim &claim) override {
return remove(claim);
}
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool remove(const minifi::ResourceClaim &claim);
+ bool remove(const minifi::ResourceClaim &claim) override;
+
+ private:
+ void run() override {
+ }
- protected:
- virtual void start();
+ std::thread& getThread() override {
+ return thread_;
+ }
Review Comment:
Done.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r917982943
##########
controller/Controller.h:
##########
@@ -246,15 +247,24 @@ std::shared_ptr<org::apache::nifi::minifi::core::controller::ControllerService>
configuration->get(org::apache::nifi::minifi::Configure::nifi_provenance_repository_class_name, prov_repo_class);
// Create repos for flow record and provenance
- const std::shared_ptr prov_repo = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const std::shared_ptr prov_repo_base = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const auto prov_repo = std::static_pointer_cast<org::apache::nifi::minifi::core::ThreadedRepository>(prov_repo_base);
Review Comment:
Good point! Done.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r932184188
##########
libminifi/include/core/repository/VolatileContentRepository.h:
##########
@@ -68,55 +59,55 @@ class VolatileContentRepository :
}
master_list_.clear();
}
+ stop();
}
/**
* Initialize the volatile content repo
* @param configure configuration
*/
- virtual bool initialize(const std::shared_ptr<Configure> &configure);
-
- /**
- * Stop any thread associated with the volatile content repository.
- */
- virtual void stop();
+ bool initialize(const std::shared_ptr<Configure> &configure) override;
/**
* Creates writable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream the consumer will write to.
*/
- virtual std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append);
+ std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append) override;
/**
* Creates readable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream from which the consumer will read..
*/
- virtual std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim);
+ std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim) override;
- virtual bool exists(const minifi::ResourceClaim &streamId);
+ bool exists(const minifi::ResourceClaim &streamId) override;
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool close(const minifi::ResourceClaim &claim) {
+ bool close(const minifi::ResourceClaim &claim) override {
return remove(claim);
}
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool remove(const minifi::ResourceClaim &claim);
+ bool remove(const minifi::ResourceClaim &claim) override;
+
+ private:
+ void run() override {
+ }
- protected:
- virtual void start();
+ std::thread& getThread() override {
+ return thread_;
+ }
Review Comment:
This makes sense, so be it.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r929113223
##########
libminifi/include/core/repository/VolatileContentRepository.h:
##########
@@ -68,55 +59,55 @@ class VolatileContentRepository :
}
master_list_.clear();
}
+ stop();
}
/**
* Initialize the volatile content repo
* @param configure configuration
*/
- virtual bool initialize(const std::shared_ptr<Configure> &configure);
-
- /**
- * Stop any thread associated with the volatile content repository.
- */
- virtual void stop();
+ bool initialize(const std::shared_ptr<Configure> &configure) override;
/**
* Creates writable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream the consumer will write to.
*/
- virtual std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append);
+ std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append) override;
/**
* Creates readable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream from which the consumer will read..
*/
- virtual std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim);
+ std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim) override;
- virtual bool exists(const minifi::ResourceClaim &streamId);
+ bool exists(const minifi::ResourceClaim &streamId) override;
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool close(const minifi::ResourceClaim &claim) {
+ bool close(const minifi::ResourceClaim &claim) override {
return remove(claim);
}
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool remove(const minifi::ResourceClaim &claim);
+ bool remove(const minifi::ResourceClaim &claim) override;
+
+ private:
+ void run() override {
+ }
- protected:
- virtual void start();
+ std::thread& getThread() override {
+ return thread_;
+ }
Review Comment:
Okay, I will partially revert to previous implementation of having a ThreadedRepository, but without exposing it anywhere. If it is possible. If I run into something that prevents it, I will update here.
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r917933827
##########
controller/Controller.h:
##########
@@ -246,15 +247,24 @@ std::shared_ptr<org::apache::nifi::minifi::core::controller::ControllerService>
configuration->get(org::apache::nifi::minifi::Configure::nifi_provenance_repository_class_name, prov_repo_class);
// Create repos for flow record and provenance
- const std::shared_ptr prov_repo = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const std::shared_ptr prov_repo_base = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const auto prov_repo = std::static_pointer_cast<org::apache::nifi::minifi::core::ThreadedRepository>(prov_repo_base);
Review Comment:
`createRepository` doesn't seem to guarantee that what it returns is a `ThreadedRepository`, a `dynamic_pointer_cast` could be more appropriate in these places?
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r923433374
##########
controller/Controller.h:
##########
@@ -246,15 +247,24 @@ std::shared_ptr<org::apache::nifi::minifi::core::controller::ControllerService>
configuration->get(org::apache::nifi::minifi::Configure::nifi_provenance_repository_class_name, prov_repo_class);
// Create repos for flow record and provenance
- const std::shared_ptr prov_repo = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const std::shared_ptr prov_repo_base = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const auto prov_repo = std::dynamic_pointer_cast<org::apache::nifi::minifi::core::ThreadedRepository>(prov_repo_base);
Review Comment:
`core::createRepository` is no longer existing
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r924437352
##########
libminifi/include/core/RepositoryFactory.h:
##########
@@ -49,10 +45,14 @@ std::unique_ptr<core::Repository> createRepository(const std::string& configurat
*/
std::unique_ptr<core::ContentRepository> createContentRepository(const std::string& configuration_class_name, bool fail_safe = false, const std::string& repo_name = "");
-} // namespace core
-} // namespace minifi
-} // namespace nifi
-} // namespace apache
-} // namespace org
+/**
+ * Create a threaded repository represented by the configuration class name
+ * @param configuration_class_name configuration class name
+ * @param fail_safe determines whether or not to make the default class if configuration_class_name is invalid
+ * @param repo_name name of the repository
+ */
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, const std::string& repo_name = "");
Review Comment:
They are merged now.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r924508317
##########
controller/Controller.h:
##########
@@ -246,15 +247,25 @@ std::shared_ptr<org::apache::nifi::minifi::core::controller::ControllerService>
configuration->get(org::apache::nifi::minifi::Configure::nifi_provenance_repository_class_name, prov_repo_class);
// Create repos for flow record and provenance
- const std::shared_ptr prov_repo = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const std::shared_ptr prov_repo_base = org::apache::nifi::minifi::core::createRepository(prov_repo_class,
+ "provenance");
+ const auto prov_repo = std::dynamic_pointer_cast<org::apache::nifi::minifi::core::Repository>(prov_repo_base);
+ if (!prov_repo) {
+ throw org::apache::nifi::minifi::Exception(org::apache::nifi::minifi::REPOSITORY_EXCEPTION, "Could not create provenance repository");
+ }
prov_repo->initialize(configuration);
configuration->get(org::apache::nifi::minifi::Configure::nifi_flow_repository_class_name, flow_repo_class);
- const std::shared_ptr flow_repo = org::apache::nifi::minifi::core::createRepository(flow_repo_class, true, "flowfile");
+ const std::shared_ptr flow_repo_base = org::apache::nifi::minifi::core::createRepository(flow_repo_class, "flowfile");
+ const auto flow_repo = std::dynamic_pointer_cast<org::apache::nifi::minifi::core::Repository>(flow_repo_base);
Review Comment:
Done.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r934658310
##########
libminifi/include/core/Repository.h:
##########
@@ -218,56 +195,77 @@ class Repository : public virtual core::SerializableComponent, public core::Trac
return Put(key, buffer, bufferSize);
}
- uint64_t incrementSize(const char* /*fpath*/, const struct stat *sb, int /*typeflag*/) {
- return (repo_size_ += sb->st_size);
- }
-
virtual void loadComponent(const std::shared_ptr<core::ContentRepository>& /*content_repo*/) {
}
- virtual uint64_t getRepoSize();
+ virtual uint64_t getRepoSize() const {
+ return repo_size_;
+ }
std::string getDirectory() const {
return directory_;
}
- // Prevent default copy constructor and assignment operation
- // Only support pass by reference or pointer
- Repository(const Repository &parent) = delete;
- Repository &operator=(const Repository &parent) = delete;
+ // Starts repository monitor thread
+ void start() {
+ if (running_.exchange(true)) {
+ return;
+ }
+ if (purge_period_ <= std::chrono::milliseconds(0)) {
+ return;
+ }
+ getThread() = std::thread(&Repository::run, this);
+
+ logger_->log_debug("%s Repository monitor thread start", name_);
+ }
+
+ // Stops repository monitor thread
+ void stop() {
+ if (!running_.exchange(false)) {
+ return;
+ }
+ if (getThread().joinable()) {
+ getThread().join();
+ }
+ logger_->log_debug("%s Repository monitor thread stop", name_);
+ }
Review Comment:
Done.
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r932163531
##########
libminifi/include/core/repository/VolatileContentRepository.h:
##########
@@ -68,55 +59,55 @@ class VolatileContentRepository :
}
master_list_.clear();
}
+ stop();
}
/**
* Initialize the volatile content repo
* @param configure configuration
*/
- virtual bool initialize(const std::shared_ptr<Configure> &configure);
-
- /**
- * Stop any thread associated with the volatile content repository.
- */
- virtual void stop();
+ bool initialize(const std::shared_ptr<Configure> &configure) override;
/**
* Creates writable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream the consumer will write to.
*/
- virtual std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append);
+ std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append) override;
/**
* Creates readable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream from which the consumer will read..
*/
- virtual std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim);
+ std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim) override;
- virtual bool exists(const minifi::ResourceClaim &streamId);
+ bool exists(const minifi::ResourceClaim &streamId) override;
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool close(const minifi::ResourceClaim &claim) {
+ bool close(const minifi::ResourceClaim &claim) override {
return remove(claim);
}
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool remove(const minifi::ResourceClaim &claim);
+ bool remove(const minifi::ResourceClaim &claim) override;
+
+ private:
+ void run() override {
+ }
- protected:
- virtual void start();
+ std::thread& getThread() override {
+ return thread_;
+ }
Review Comment:
I think a pure virtual `start` on `Repository` would be a valid approach, I could imagine repositories not needing a separate thread, but still needing start/stop logic, e.g. a synchronous DB repo. We don't have any of those as far as I'm aware, I'm just justifying keeping `start` in the `Repostory` base class. What do you think?
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r925832155
##########
libminifi/include/core/repository/VolatileContentRepository.h:
##########
@@ -68,55 +59,55 @@ class VolatileContentRepository :
}
master_list_.clear();
}
+ stop();
}
/**
* Initialize the volatile content repo
* @param configure configuration
*/
- virtual bool initialize(const std::shared_ptr<Configure> &configure);
-
- /**
- * Stop any thread associated with the volatile content repository.
- */
- virtual void stop();
+ bool initialize(const std::shared_ptr<Configure> &configure) override;
/**
* Creates writable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream the consumer will write to.
*/
- virtual std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append);
+ std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim, bool append) override;
/**
* Creates readable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream from which the consumer will read..
*/
- virtual std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim);
+ std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim) override;
- virtual bool exists(const minifi::ResourceClaim &streamId);
+ bool exists(const minifi::ResourceClaim &streamId) override;
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool close(const minifi::ResourceClaim &claim) {
+ bool close(const minifi::ResourceClaim &claim) override {
return remove(claim);
}
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in volatile memory.
*/
- virtual bool remove(const minifi::ResourceClaim &claim);
+ bool remove(const minifi::ResourceClaim &claim) override;
+
+ private:
+ void run() override {
+ }
- protected:
- virtual void start();
+ std::thread& getThread() override {
+ return thread_;
+ }
Review Comment:
This class doesn't need a thread. Can you think of a structure that doesn't require all repositories to have a thread?
##########
libminifi/include/core/Repository.h:
##########
@@ -218,56 +195,77 @@ class Repository : public virtual core::SerializableComponent, public core::Trac
return Put(key, buffer, bufferSize);
}
- uint64_t incrementSize(const char* /*fpath*/, const struct stat *sb, int /*typeflag*/) {
- return (repo_size_ += sb->st_size);
- }
-
virtual void loadComponent(const std::shared_ptr<core::ContentRepository>& /*content_repo*/) {
}
- virtual uint64_t getRepoSize();
+ virtual uint64_t getRepoSize() const {
+ return repo_size_;
+ }
std::string getDirectory() const {
return directory_;
}
- // Prevent default copy constructor and assignment operation
- // Only support pass by reference or pointer
- Repository(const Repository &parent) = delete;
- Repository &operator=(const Repository &parent) = delete;
+ // Starts repository monitor thread
+ void start() {
+ if (running_.exchange(true)) {
+ return;
+ }
+ if (purge_period_ <= std::chrono::milliseconds(0)) {
+ return;
+ }
+ getThread() = std::thread(&Repository::run, this);
+
+ logger_->log_debug("%s Repository monitor thread start", name_);
+ }
+
+ // Stops repository monitor thread
+ void stop() {
+ if (!running_.exchange(false)) {
+ return;
+ }
+ if (getThread().joinable()) {
+ getThread().join();
+ }
+ logger_->log_debug("%s Repository monitor thread stop", name_);
+ }
Review Comment:
It's possible to start again on another thread after line 224 has finished, and we're waiting for 227 or 228. One should not be able to start the repository until stopping has finished. Same with start, you shouldn't be able to stop until starting has finished.
I suggest using a state enum with 4 values: starting, running, stopping, stopped.
Only transitions to the right and stopped -> starting should be 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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r871108878
##########
extensions/rocksdb-repos/ProvenanceRepository.h:
##########
@@ -26,35 +26,31 @@
#include "rocksdb/db.h"
#include "rocksdb/options.h"
#include "rocksdb/slice.h"
-#include "core/Repository.h"
#include "core/Core.h"
-#include "provenance/Provenance.h"
#include "core/logging/LoggerConfiguration.h"
+#include "core/ThreadedRepository.h"
+#include "provenance/Provenance.h"
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace provenance {
+namespace org::apache::nifi::minifi::provenance {
-#define PROVENANCE_DIRECTORY "./provenance_repository"
-#define MAX_PROVENANCE_STORAGE_SIZE (10*1024*1024) // 10M
+constexpr auto PROVENANCE_DIRECTORY = "./provenance_repository";
+constexpr auto MAX_PROVENANCE_STORAGE_SIZE = 10 * 1024 * 1024; // 10M
constexpr auto MAX_PROVENANCE_ENTRY_LIFE_TIME = std::chrono::minutes(1);
constexpr auto PROVENANCE_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class ProvenanceRepository : public core::Repository {
+class ProvenanceRepository : public core::ThreadedRepository {
public:
- ProvenanceRepository(const std::string& name, const utils::Identifier& /*uuid*/)
- : ProvenanceRepository(name) {
- }
- // Constructor
- /*!
- * Create a new provenance repository
- */
- explicit ProvenanceRepository(const std::string& repo_name = "", std::string directory = PROVENANCE_DIRECTORY, std::chrono::milliseconds maxPartitionMillis = MAX_PROVENANCE_ENTRY_LIFE_TIME,
- int64_t maxPartitionBytes = MAX_PROVENANCE_STORAGE_SIZE, std::chrono::milliseconds purgePeriod = PROVENANCE_PURGE_PERIOD)
- : core::SerializableComponent(repo_name),
- Repository(repo_name.length() > 0 ? repo_name : core::getClassName<ProvenanceRepository>(), directory, maxPartitionMillis, maxPartitionBytes, purgePeriod) {
+ ProvenanceRepository(const std::string &name, const utils::Identifier & /*uuid*/)
+ : ProvenanceRepository(name) {
+ }
+
+ explicit ProvenanceRepository(const std::string &repo_name = "", std::string directory = PROVENANCE_DIRECTORY,
+ std::chrono::milliseconds maxPartitionMillis = MAX_PROVENANCE_ENTRY_LIFE_TIME,
+ int64_t maxPartitionBytes = MAX_PROVENANCE_STORAGE_SIZE,
+ std::chrono::milliseconds purgePeriod = PROVENANCE_PURGE_PERIOD)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name.length() > 0 ? repo_name : core::getClassName<ProvenanceRepository>(), directory,
+ maxPartitionMillis, maxPartitionBytes, purgePeriod) {
Review Comment:
Just a nitpick, but I don't think deeper alignment of this line is necessary
##########
extensions/rocksdb-repos/FlowFileRepository.h:
##########
@@ -27,30 +27,25 @@
#include "rocksdb/options.h"
#include "rocksdb/slice.h"
#include "rocksdb/utilities/checkpoint.h"
-#include "core/Repository.h"
#include "core/Core.h"
-#include "Connection.h"
#include "core/logging/LoggerConfiguration.h"
+#include "core/ThreadedRepository.h"
+#include "Connection.h"
#include "concurrentqueue.h"
#include "database/RocksDatabase.h"
#include "encryption/RocksDbEncryptionProvider.h"
#include "utils/crypto/EncryptionProvider.h"
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace core {
-namespace repository {
+namespace org::apache::nifi::minifi::core::repository {
#ifdef WIN32
-#define FLOWFILE_REPOSITORY_DIRECTORY ".\\flowfile_repository"
-#define FLOWFILE_CHECKPOINT_DIRECTORY ".\\flowfile_checkpoint"
+constexpr auto FLOWFILE_REPOSITORY_DIRECTORY = ".\\flowfile_repository";
+constexpr auto FLOWFILE_CHECKPOINT_DIRECTORY = ".\\flowfile_checkpoint";
#else
-#define FLOWFILE_REPOSITORY_DIRECTORY "./flowfile_repository"
-#define FLOWFILE_CHECKPOINT_DIRECTORY "./flowfile_checkpoint"
+constexpr auto FLOWFILE_REPOSITORY_DIRECTORY = "./flowfile_repository";
+constexpr auto FLOWFILE_CHECKPOINT_DIRECTORY = "./flowfile_checkpoint";
#endif
-#define MAX_FLOWFILE_REPOSITORY_STORAGE_SIZE (10*1024*1024) // 10M
+constexpr auto MAX_FLOWFILE_REPOSITORY_STORAGE_SIZE = 10*1024*1024; // 10M
Review Comment:
You could use 10_MiB from Literals.h. The same could be used in ProvenanceRepository.h and Repository.h
##########
libminifi/src/core/RepositoryFactory.cpp:
##########
@@ -96,6 +96,62 @@ std::unique_ptr<core::ContentRepository> createContentRepository(const std::stri
throw std::runtime_error("Support for the provided configuration class could not be found");
}
+class NoOpThreadedRepository : public core::ThreadedRepository {
+ public:
+ explicit NoOpThreadedRepository(const std::string& repo_name)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name) {
+ }
+
+ ~NoOpThreadedRepository() override {
+ stop();
+ }
+
+ private:
+ void run() override {
+ }
+
+ std::thread& getThread() override {
+ return thread_;
+ }
+
+ std::thread thread_;
+};
+
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, bool fail_safe, const std::string& repo_name) {
Review Comment:
Do we need the fail_safe argument? It seems that in all scenarios this is set to true, so we could just suppress the exceptions in all cases and remove this parameter.
##########
libminifi/src/core/RepositoryFactory.cpp:
##########
@@ -96,6 +96,62 @@ std::unique_ptr<core::ContentRepository> createContentRepository(const std::stri
throw std::runtime_error("Support for the provided configuration class could not be found");
}
+class NoOpThreadedRepository : public core::ThreadedRepository {
+ public:
+ explicit NoOpThreadedRepository(const std::string& repo_name)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name) {
+ }
+
+ ~NoOpThreadedRepository() override {
+ stop();
+ }
+
+ private:
+ void run() override {
+ }
+
+ std::thread& getThread() override {
+ return thread_;
+ }
+
+ std::thread thread_;
+};
+
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, bool fail_safe, const std::string& repo_name) {
+ std::string class_name_lc = configuration_class_name;
+ std::transform(class_name_lc.begin(), class_name_lc.end(), class_name_lc.begin(), ::tolower);
+ try {
+ auto return_obj = core::ClassLoader::getDefaultClassLoader().instantiate<core::ThreadedRepository>(class_name_lc, class_name_lc);
+ if (return_obj) {
+ return_obj->setName(repo_name);
+ return return_obj;
+ }
+ // if the desired repos don't exist, we can try doing string matches and rely on volatile repositories
+ if (class_name_lc == "flowfilerepository" || class_name_lc == "volatileflowfilerepository") {
+ return_obj = instantiate<repository::VolatileFlowFileRepository>(repo_name);
+ } else if (class_name_lc == "provenancerepository" || class_name_lc == "volatileprovenancefilerepository") {
+ return_obj = instantiate<repository::VolatileProvenanceRepository>(repo_name);
+ } else if (class_name_lc == "nooprepository") {
+ return_obj = std::make_unique<core::NoOpThreadedRepository>(repo_name);
+ }
+ if (return_obj) {
+ return return_obj;
+ }
+ if (fail_safe) {
+ return {};
+ } else {
+ throw std::runtime_error("Support for the provided configuration class could not be found");
+ }
+ } catch (const std::runtime_error &) {
+ if (fail_safe) {
+ return {};
+ }
+ }
+
+ throw std::runtime_error("Support for the provided configuration class could not be found");
+}
+
} /* namespace core */
} /* namespace minifi */
} /* namespace nifi */
Review Comment:
We could concatenate the nested namespaces here as well.
##########
libminifi/include/core/Repository.h:
##########
@@ -240,30 +205,20 @@ class Repository : public virtual core::SerializableComponent, public core::Trac
std::map<std::string, core::Connectable*> containers_;
std::map<std::string, core::Connectable*> connection_map_;
- // Mutex for protection
- std::mutex mutex_;
// repository directory
Review Comment:
You could remove this and similar uninformative member comments below
##########
libminifi/include/core/repository/VolatileRepository.h:
##########
@@ -39,31 +39,24 @@ namespace nifi {
namespace minifi {
namespace core {
namespace repository {
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Woverloaded-virtual"
-#elif defined(__GNUC__) || defined(__GNUG__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Woverloaded-virtual"
-#endif
+
/**
* Flow File repository
* Design: Extends Repository and implements the run function, using RocksDB as the primary substrate.
*/
-template<typename T>
-class VolatileRepository : public core::Repository {
+template<typename T, typename T_Repository = core::Repository>
Review Comment:
Maybe you could rename the template arguments to KeyType and RepositoryType to be more clear
##########
libminifi/src/core/RepositoryFactory.cpp:
##########
@@ -96,6 +96,62 @@ std::unique_ptr<core::ContentRepository> createContentRepository(const std::stri
throw std::runtime_error("Support for the provided configuration class could not be found");
}
+class NoOpThreadedRepository : public core::ThreadedRepository {
+ public:
+ explicit NoOpThreadedRepository(const std::string& repo_name)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name) {
+ }
+
+ ~NoOpThreadedRepository() override {
+ stop();
+ }
+
+ private:
+ void run() override {
+ }
+
+ std::thread& getThread() override {
+ return thread_;
+ }
+
+ std::thread thread_;
+};
+
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, bool fail_safe, const std::string& repo_name) {
+ std::string class_name_lc = configuration_class_name;
+ std::transform(class_name_lc.begin(), class_name_lc.end(), class_name_lc.begin(), ::tolower);
+ try {
+ auto return_obj = core::ClassLoader::getDefaultClassLoader().instantiate<core::ThreadedRepository>(class_name_lc, class_name_lc);
+ if (return_obj) {
+ return_obj->setName(repo_name);
+ return return_obj;
+ }
+ // if the desired repos don't exist, we can try doing string matches and rely on volatile repositories
+ if (class_name_lc == "flowfilerepository" || class_name_lc == "volatileflowfilerepository") {
+ return_obj = instantiate<repository::VolatileFlowFileRepository>(repo_name);
+ } else if (class_name_lc == "provenancerepository" || class_name_lc == "volatileprovenancefilerepository") {
+ return_obj = instantiate<repository::VolatileProvenanceRepository>(repo_name);
+ } else if (class_name_lc == "nooprepository") {
+ return_obj = std::make_unique<core::NoOpThreadedRepository>(repo_name);
+ }
+ if (return_obj) {
+ return return_obj;
+ }
+ if (fail_safe) {
+ return {};
+ } else {
+ throw std::runtime_error("Support for the provided configuration class could not be found");
+ }
+ } catch (const std::runtime_error &) {
+ if (fail_safe) {
+ return {};
+ }
+ }
+
+ throw std::runtime_error("Support for the provided configuration class could not be found");
Review Comment:
We don't need to throw here if we just call a `throw` in the catch block rethrowing the original exception.
##########
libminifi/include/core/Repository.h:
##########
@@ -240,30 +205,20 @@ class Repository : public virtual core::SerializableComponent, public core::Trac
std::map<std::string, core::Connectable*> containers_;
std::map<std::string, core::Connectable*> connection_map_;
- // Mutex for protection
- std::mutex mutex_;
// repository directory
std::string directory_;
- // max db entry life time
+ // max db entry lifetime
std::chrono::milliseconds max_partition_millis_;
// max db size
int64_t max_partition_bytes_;
// purge period
std::chrono::milliseconds purge_period_;
- // thread
- std::thread thread_;
- // whether the monitoring thread is running for the repo while it was enabled
- std::atomic<bool> running_;
- // whether stop accepting provenace event
+ // whether stop accepting provenance event
Review Comment:
Typo: Whether TO stop...
##########
libminifi/src/core/RepositoryFactory.cpp:
##########
@@ -96,6 +96,62 @@ std::unique_ptr<core::ContentRepository> createContentRepository(const std::stri
throw std::runtime_error("Support for the provided configuration class could not be found");
}
+class NoOpThreadedRepository : public core::ThreadedRepository {
+ public:
+ explicit NoOpThreadedRepository(const std::string& repo_name)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name) {
+ }
+
+ ~NoOpThreadedRepository() override {
+ stop();
+ }
+
+ private:
+ void run() override {
+ }
+
+ std::thread& getThread() override {
+ return thread_;
+ }
+
+ std::thread thread_;
+};
+
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, bool fail_safe, const std::string& repo_name) {
+ std::string class_name_lc = configuration_class_name;
+ std::transform(class_name_lc.begin(), class_name_lc.end(), class_name_lc.begin(), ::tolower);
+ try {
+ auto return_obj = core::ClassLoader::getDefaultClassLoader().instantiate<core::ThreadedRepository>(class_name_lc, class_name_lc);
+ if (return_obj) {
+ return_obj->setName(repo_name);
+ return return_obj;
+ }
+ // if the desired repos don't exist, we can try doing string matches and rely on volatile repositories
+ if (class_name_lc == "flowfilerepository" || class_name_lc == "volatileflowfilerepository") {
+ return_obj = instantiate<repository::VolatileFlowFileRepository>(repo_name);
+ } else if (class_name_lc == "provenancerepository" || class_name_lc == "volatileprovenancefilerepository") {
+ return_obj = instantiate<repository::VolatileProvenanceRepository>(repo_name);
+ } else if (class_name_lc == "nooprepository") {
+ return_obj = std::make_unique<core::NoOpThreadedRepository>(repo_name);
+ }
+ if (return_obj) {
+ return return_obj;
+ }
Review Comment:
I don't think we need this, we could just return the repositories in each branch.
--
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] adam-markovics commented on a diff in pull request #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r873907644
##########
libminifi/src/core/RepositoryFactory.cpp:
##########
@@ -96,6 +96,62 @@ std::unique_ptr<core::ContentRepository> createContentRepository(const std::stri
throw std::runtime_error("Support for the provided configuration class could not be found");
}
+class NoOpThreadedRepository : public core::ThreadedRepository {
+ public:
+ explicit NoOpThreadedRepository(const std::string& repo_name)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name) {
+ }
+
+ ~NoOpThreadedRepository() override {
+ stop();
+ }
+
+ private:
+ void run() override {
+ }
+
+ std::thread& getThread() override {
+ return thread_;
+ }
+
+ std::thread thread_;
+};
+
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, bool fail_safe, const std::string& repo_name) {
+ std::string class_name_lc = configuration_class_name;
+ std::transform(class_name_lc.begin(), class_name_lc.end(), class_name_lc.begin(), ::tolower);
+ try {
+ auto return_obj = core::ClassLoader::getDefaultClassLoader().instantiate<core::ThreadedRepository>(class_name_lc, class_name_lc);
+ if (return_obj) {
+ return_obj->setName(repo_name);
+ return return_obj;
+ }
+ // if the desired repos don't exist, we can try doing string matches and rely on volatile repositories
+ if (class_name_lc == "flowfilerepository" || class_name_lc == "volatileflowfilerepository") {
+ return_obj = instantiate<repository::VolatileFlowFileRepository>(repo_name);
+ } else if (class_name_lc == "provenancerepository" || class_name_lc == "volatileprovenancefilerepository") {
+ return_obj = instantiate<repository::VolatileProvenanceRepository>(repo_name);
+ } else if (class_name_lc == "nooprepository") {
+ return_obj = std::make_unique<core::NoOpThreadedRepository>(repo_name);
+ }
+ if (return_obj) {
+ return return_obj;
+ }
+ if (fail_safe) {
+ return {};
+ } else {
+ throw std::runtime_error("Support for the provided configuration class could not be found");
+ }
+ } catch (const std::runtime_error &) {
+ if (fail_safe) {
+ return {};
+ }
+ }
+
+ throw std::runtime_error("Support for the provided configuration class could not be found");
+}
+
} /* namespace core */
} /* namespace minifi */
} /* namespace nifi */
Review Comment:
Done.
##########
libminifi/src/core/RepositoryFactory.cpp:
##########
@@ -96,6 +96,62 @@ std::unique_ptr<core::ContentRepository> createContentRepository(const std::stri
throw std::runtime_error("Support for the provided configuration class could not be found");
}
+class NoOpThreadedRepository : public core::ThreadedRepository {
+ public:
+ explicit NoOpThreadedRepository(const std::string& repo_name)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name) {
+ }
+
+ ~NoOpThreadedRepository() override {
+ stop();
+ }
+
+ private:
+ void run() override {
+ }
+
+ std::thread& getThread() override {
+ return thread_;
+ }
+
+ std::thread thread_;
+};
+
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, bool fail_safe, const std::string& repo_name) {
Review Comment:
Done.
##########
libminifi/src/core/RepositoryFactory.cpp:
##########
@@ -96,6 +96,62 @@ std::unique_ptr<core::ContentRepository> createContentRepository(const std::stri
throw std::runtime_error("Support for the provided configuration class could not be found");
}
+class NoOpThreadedRepository : public core::ThreadedRepository {
+ public:
+ explicit NoOpThreadedRepository(const std::string& repo_name)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name) {
+ }
+
+ ~NoOpThreadedRepository() override {
+ stop();
+ }
+
+ private:
+ void run() override {
+ }
+
+ std::thread& getThread() override {
+ return thread_;
+ }
+
+ std::thread thread_;
+};
+
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, bool fail_safe, const std::string& repo_name) {
+ std::string class_name_lc = configuration_class_name;
+ std::transform(class_name_lc.begin(), class_name_lc.end(), class_name_lc.begin(), ::tolower);
+ try {
+ auto return_obj = core::ClassLoader::getDefaultClassLoader().instantiate<core::ThreadedRepository>(class_name_lc, class_name_lc);
+ if (return_obj) {
+ return_obj->setName(repo_name);
+ return return_obj;
+ }
+ // if the desired repos don't exist, we can try doing string matches and rely on volatile repositories
+ if (class_name_lc == "flowfilerepository" || class_name_lc == "volatileflowfilerepository") {
+ return_obj = instantiate<repository::VolatileFlowFileRepository>(repo_name);
+ } else if (class_name_lc == "provenancerepository" || class_name_lc == "volatileprovenancefilerepository") {
+ return_obj = instantiate<repository::VolatileProvenanceRepository>(repo_name);
+ } else if (class_name_lc == "nooprepository") {
+ return_obj = std::make_unique<core::NoOpThreadedRepository>(repo_name);
+ }
+ if (return_obj) {
+ return return_obj;
+ }
+ if (fail_safe) {
+ return {};
+ } else {
+ throw std::runtime_error("Support for the provided configuration class could not be found");
+ }
+ } catch (const std::runtime_error &) {
+ if (fail_safe) {
+ return {};
+ }
+ }
+
+ throw std::runtime_error("Support for the provided configuration class could not be found");
Review Comment:
Done.
##########
libminifi/src/core/RepositoryFactory.cpp:
##########
@@ -96,6 +96,62 @@ std::unique_ptr<core::ContentRepository> createContentRepository(const std::stri
throw std::runtime_error("Support for the provided configuration class could not be found");
}
+class NoOpThreadedRepository : public core::ThreadedRepository {
+ public:
+ explicit NoOpThreadedRepository(const std::string& repo_name)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name) {
+ }
+
+ ~NoOpThreadedRepository() override {
+ stop();
+ }
+
+ private:
+ void run() override {
+ }
+
+ std::thread& getThread() override {
+ return thread_;
+ }
+
+ std::thread thread_;
+};
+
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, bool fail_safe, const std::string& repo_name) {
+ std::string class_name_lc = configuration_class_name;
+ std::transform(class_name_lc.begin(), class_name_lc.end(), class_name_lc.begin(), ::tolower);
+ try {
+ auto return_obj = core::ClassLoader::getDefaultClassLoader().instantiate<core::ThreadedRepository>(class_name_lc, class_name_lc);
+ if (return_obj) {
+ return_obj->setName(repo_name);
+ return return_obj;
+ }
+ // if the desired repos don't exist, we can try doing string matches and rely on volatile repositories
+ if (class_name_lc == "flowfilerepository" || class_name_lc == "volatileflowfilerepository") {
+ return_obj = instantiate<repository::VolatileFlowFileRepository>(repo_name);
+ } else if (class_name_lc == "provenancerepository" || class_name_lc == "volatileprovenancefilerepository") {
+ return_obj = instantiate<repository::VolatileProvenanceRepository>(repo_name);
+ } else if (class_name_lc == "nooprepository") {
+ return_obj = std::make_unique<core::NoOpThreadedRepository>(repo_name);
+ }
+ if (return_obj) {
+ return return_obj;
+ }
Review Comment:
Done.
##########
libminifi/include/core/repository/VolatileRepository.h:
##########
@@ -39,31 +39,24 @@ namespace nifi {
namespace minifi {
namespace core {
namespace repository {
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Woverloaded-virtual"
-#elif defined(__GNUC__) || defined(__GNUG__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Woverloaded-virtual"
-#endif
+
/**
* Flow File repository
* Design: Extends Repository and implements the run function, using RocksDB as the primary substrate.
*/
-template<typename T>
-class VolatileRepository : public core::Repository {
+template<typename T, typename T_Repository = core::Repository>
Review Comment:
Done.
##########
libminifi/include/core/Repository.h:
##########
@@ -240,30 +205,20 @@ class Repository : public virtual core::SerializableComponent, public core::Trac
std::map<std::string, core::Connectable*> containers_;
std::map<std::string, core::Connectable*> connection_map_;
- // Mutex for protection
- std::mutex mutex_;
// repository directory
Review Comment:
Done.
##########
libminifi/include/core/Repository.h:
##########
@@ -240,30 +205,20 @@ class Repository : public virtual core::SerializableComponent, public core::Trac
std::map<std::string, core::Connectable*> containers_;
std::map<std::string, core::Connectable*> connection_map_;
- // Mutex for protection
- std::mutex mutex_;
// repository directory
std::string directory_;
- // max db entry life time
+ // max db entry lifetime
std::chrono::milliseconds max_partition_millis_;
// max db size
int64_t max_partition_bytes_;
// purge period
std::chrono::milliseconds purge_period_;
- // thread
- std::thread thread_;
- // whether the monitoring thread is running for the repo while it was enabled
- std::atomic<bool> running_;
- // whether stop accepting provenace event
+ // whether stop accepting provenance event
Review Comment:
Done.
##########
extensions/rocksdb-repos/ProvenanceRepository.h:
##########
@@ -26,35 +26,31 @@
#include "rocksdb/db.h"
#include "rocksdb/options.h"
#include "rocksdb/slice.h"
-#include "core/Repository.h"
#include "core/Core.h"
-#include "provenance/Provenance.h"
#include "core/logging/LoggerConfiguration.h"
+#include "core/ThreadedRepository.h"
+#include "provenance/Provenance.h"
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace provenance {
+namespace org::apache::nifi::minifi::provenance {
-#define PROVENANCE_DIRECTORY "./provenance_repository"
-#define MAX_PROVENANCE_STORAGE_SIZE (10*1024*1024) // 10M
+constexpr auto PROVENANCE_DIRECTORY = "./provenance_repository";
+constexpr auto MAX_PROVENANCE_STORAGE_SIZE = 10 * 1024 * 1024; // 10M
constexpr auto MAX_PROVENANCE_ENTRY_LIFE_TIME = std::chrono::minutes(1);
constexpr auto PROVENANCE_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class ProvenanceRepository : public core::Repository {
+class ProvenanceRepository : public core::ThreadedRepository {
public:
- ProvenanceRepository(const std::string& name, const utils::Identifier& /*uuid*/)
- : ProvenanceRepository(name) {
- }
- // Constructor
- /*!
- * Create a new provenance repository
- */
- explicit ProvenanceRepository(const std::string& repo_name = "", std::string directory = PROVENANCE_DIRECTORY, std::chrono::milliseconds maxPartitionMillis = MAX_PROVENANCE_ENTRY_LIFE_TIME,
- int64_t maxPartitionBytes = MAX_PROVENANCE_STORAGE_SIZE, std::chrono::milliseconds purgePeriod = PROVENANCE_PURGE_PERIOD)
- : core::SerializableComponent(repo_name),
- Repository(repo_name.length() > 0 ? repo_name : core::getClassName<ProvenanceRepository>(), directory, maxPartitionMillis, maxPartitionBytes, purgePeriod) {
+ ProvenanceRepository(const std::string &name, const utils::Identifier & /*uuid*/)
+ : ProvenanceRepository(name) {
+ }
+
+ explicit ProvenanceRepository(const std::string &repo_name = "", std::string directory = PROVENANCE_DIRECTORY,
+ std::chrono::milliseconds maxPartitionMillis = MAX_PROVENANCE_ENTRY_LIFE_TIME,
+ int64_t maxPartitionBytes = MAX_PROVENANCE_STORAGE_SIZE,
+ std::chrono::milliseconds purgePeriod = PROVENANCE_PURGE_PERIOD)
+ : core::SerializableComponent(repo_name),
+ ThreadedRepository(repo_name.length() > 0 ? repo_name : core::getClassName<ProvenanceRepository>(), directory,
+ maxPartitionMillis, maxPartitionBytes, purgePeriod) {
Review Comment:
Done.
##########
extensions/rocksdb-repos/FlowFileRepository.h:
##########
@@ -27,30 +27,25 @@
#include "rocksdb/options.h"
#include "rocksdb/slice.h"
#include "rocksdb/utilities/checkpoint.h"
-#include "core/Repository.h"
#include "core/Core.h"
-#include "Connection.h"
#include "core/logging/LoggerConfiguration.h"
+#include "core/ThreadedRepository.h"
+#include "Connection.h"
#include "concurrentqueue.h"
#include "database/RocksDatabase.h"
#include "encryption/RocksDbEncryptionProvider.h"
#include "utils/crypto/EncryptionProvider.h"
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace core {
-namespace repository {
+namespace org::apache::nifi::minifi::core::repository {
#ifdef WIN32
-#define FLOWFILE_REPOSITORY_DIRECTORY ".\\flowfile_repository"
-#define FLOWFILE_CHECKPOINT_DIRECTORY ".\\flowfile_checkpoint"
+constexpr auto FLOWFILE_REPOSITORY_DIRECTORY = ".\\flowfile_repository";
+constexpr auto FLOWFILE_CHECKPOINT_DIRECTORY = ".\\flowfile_checkpoint";
#else
-#define FLOWFILE_REPOSITORY_DIRECTORY "./flowfile_repository"
-#define FLOWFILE_CHECKPOINT_DIRECTORY "./flowfile_checkpoint"
+constexpr auto FLOWFILE_REPOSITORY_DIRECTORY = "./flowfile_repository";
+constexpr auto FLOWFILE_CHECKPOINT_DIRECTORY = "./flowfile_checkpoint";
#endif
-#define MAX_FLOWFILE_REPOSITORY_STORAGE_SIZE (10*1024*1024) // 10M
+constexpr auto MAX_FLOWFILE_REPOSITORY_STORAGE_SIZE = 10*1024*1024; // 10M
Review Comment:
Done.
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r919232606
##########
libminifi/include/core/ThreadedRepository.h:
##########
@@ -0,0 +1,86 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <memory>
+#include <thread>
+
+#include "Repository.h"
+#include "TraceableResource.h"
+
+namespace org::apache::nifi::minifi::core {
+
+class ThreadedRepository : public core::Repository, public core::TraceableResource {
+ public:
+ using Repository::Repository;
+
+ ~ThreadedRepository() override {
+ if (running_.load()) {
+ logger_->log_error("Thread of %s should have been stopped in subclass before ThreadedRepository's destruction", name_);
+ }
+ }
+
+ // Starts repository monitor thread
+ void start() {
+ if (running_.exchange(true)) {
+ return;
+ }
+ if (purge_period_ <= std::chrono::milliseconds(0)) {
+ return;
+ }
+ getThread() = std::thread(&ThreadedRepository::run, this);
Review Comment:
You're right. I didn't find anything in the standard that disallows this, and the member function pointer explanation in [this article](https://www.codeproject.com/Articles/7150/Member-Function-Pointers-and-the-Fastest-Possible) suggests that they keep track internally whether it's virtual or non-virtual and single or multiple inheritance.
I'd still replace it with a lambda, simply because that looks simpler to me, but it's OK as is.
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r923459619
##########
libminifi/include/core/Repository.h:
##########
@@ -20,89 +20,77 @@
#ifndef LIBMINIFI_INCLUDE_CORE_REPOSITORY_H_
#define LIBMINIFI_INCLUDE_CORE_REPOSITORY_H_
-#include <memory>
-#include <utility>
#include <atomic>
#include <cstdint>
#include <cstring>
-#include <iostream>
#include <map>
+#include <memory>
#include <set>
#include <string>
-#include <thread>
+#include <utility>
#include <vector>
+
+#include "Core.h"
+#include "ResourceClaim.h"
+#include "core/Connectable.h"
#include "core/ContentRepository.h"
+#include "core/Property.h"
#include "core/SerializableComponent.h"
-#include "properties/Configure.h"
#include "core/logging/LoggerConfiguration.h"
-#include "core/Property.h"
-#include "ResourceClaim.h"
-#include "utils/TimeUtil.h"
-#include "utils/StringUtils.h"
-#include "Core.h"
-#include "core/Connectable.h"
-#include "core/TraceableResource.h"
+#include "properties/Configure.h"
#include "utils/BackTrace.h"
+#include "utils/Literals.h"
+#include "utils/StringUtils.h"
+#include "utils/TimeUtil.h"
#ifndef WIN32
#include <sys/stat.h>
#endif
namespace org::apache::nifi::minifi::core {
-#define REPOSITORY_DIRECTORY "./repo"
-#define MAX_REPOSITORY_STORAGE_SIZE (10*1024*1024) // 10M
+constexpr auto REPOSITORY_DIRECTORY = "./repo";
+constexpr auto MAX_REPOSITORY_STORAGE_SIZE = 10_MiB;
constexpr auto MAX_REPOSITORY_ENTRY_LIFE_TIME = std::chrono::minutes(10);
constexpr auto REPOSITORY_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class Repository : public virtual core::SerializableComponent, public core::TraceableResource {
+class Repository : public virtual core::SerializableComponent {
Review Comment:
Why not all of those that don't have a _real_ default implementation?
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r924492992
##########
controller/Controller.h:
##########
@@ -246,15 +247,25 @@ std::shared_ptr<org::apache::nifi::minifi::core::controller::ControllerService>
configuration->get(org::apache::nifi::minifi::Configure::nifi_provenance_repository_class_name, prov_repo_class);
// Create repos for flow record and provenance
- const std::shared_ptr prov_repo = org::apache::nifi::minifi::core::createRepository(prov_repo_class, true, "provenance");
+ const std::shared_ptr prov_repo_base = org::apache::nifi::minifi::core::createRepository(prov_repo_class,
+ "provenance");
+ const auto prov_repo = std::dynamic_pointer_cast<org::apache::nifi::minifi::core::Repository>(prov_repo_base);
+ if (!prov_repo) {
+ throw org::apache::nifi::minifi::Exception(org::apache::nifi::minifi::REPOSITORY_EXCEPTION, "Could not create provenance repository");
+ }
prov_repo->initialize(configuration);
configuration->get(org::apache::nifi::minifi::Configure::nifi_flow_repository_class_name, flow_repo_class);
- const std::shared_ptr flow_repo = org::apache::nifi::minifi::core::createRepository(flow_repo_class, true, "flowfile");
+ const std::shared_ptr flow_repo_base = org::apache::nifi::minifi::core::createRepository(flow_repo_class, "flowfile");
+ const auto flow_repo = std::dynamic_pointer_cast<org::apache::nifi::minifi::core::Repository>(flow_repo_base);
Review Comment:
Isn't this converting to the same type now? Let's revert to just one variable, a possibly valid shared_ptr to Repository.
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r934641557
##########
libminifi/include/core/ThreadedRepository.h:
##########
@@ -0,0 +1,106 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <memory>
+#include <thread>
+
+#include "Repository.h"
+#include "TraceableResource.h"
+
+namespace org::apache::nifi::minifi::core {
+
+class ThreadedRepository : public core::Repository, public core::TraceableResource {
+ public:
+ using Repository::Repository;
+
+ ~ThreadedRepository() override {
+ if (running_state_.load() != RUNNING_STATE::STOPPED) {
+ logger_->log_error("Thread of %s should have been stopped in subclass before ThreadedRepository's destruction", name_);
+ }
+ }
+
+ bool initialize(const std::shared_ptr<Configure>& /*configure*/) override {
+ return true;
+ }
+
+ // Starts repository monitor thread
+ bool start() override {
+ // if STOPPED, turn to STARTING, otherwise return
+ RUNNING_STATE expected{RUNNING_STATE::STOPPED};
Review Comment:
I think all caps naming should be reserved for macros. Symbolic constants, including enum values are edge case, so I usually accept either, but they should be CamelCase. The type name should definitely be CamelCase, unless following some standard-like snake_case pattern from somewhere else. Please rename.
```suggestion
RunningState expected{RunningState::Stopped};
```
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r958281318
##########
libminifi/include/core/repository/FileSystemRepository.h:
##########
@@ -25,49 +25,39 @@
#include "../ContentRepository.h"
#include "properties/Configure.h"
#include "core/logging/LoggerFactory.h"
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace core {
-namespace repository {
+
+namespace org::apache::nifi::minifi::core::repository {
/**
* FileSystemRepository is a content repository that stores data onto the local file system.
*/
class FileSystemRepository : public core::ContentRepository, public core::CoreComponent {
public:
FileSystemRepository(std::string name = getClassName<FileSystemRepository>()) // NOLINT
- : core::CoreComponent(name),
- logger_(logging::LoggerFactory<FileSystemRepository>::getLogger()) {
+ : core::CoreComponent(name),
+ logger_(logging::LoggerFactory<FileSystemRepository>::getLogger()) {
Review Comment:
The old indentation was better here
--
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 #1328: MINIFICPP-1812 - Clean up Repository threads
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r923437527
##########
libminifi/include/core/RepositoryFactory.h:
##########
@@ -49,10 +45,14 @@ std::unique_ptr<core::Repository> createRepository(const std::string& configurat
*/
std::unique_ptr<core::ContentRepository> createContentRepository(const std::string& configuration_class_name, bool fail_safe = false, const std::string& repo_name = "");
-} // namespace core
-} // namespace minifi
-} // namespace nifi
-} // namespace apache
-} // namespace org
+/**
+ * Create a threaded repository represented by the configuration class name
+ * @param configuration_class_name configuration class name
+ * @param fail_safe determines whether or not to make the default class if configuration_class_name is invalid
+ * @param repo_name name of the repository
+ */
+std::unique_ptr<core::ThreadedRepository> createThreadedRepository(const std::string& configuration_class_name, const std::string& repo_name = "");
Review Comment:
In that case, maybe what is ThreadedRepository now should become just Repository, and createThreadedRepository could be renamed to createRepository?
--
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