szaszm commented on code in PR #1473:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1473#discussion_r1062417371
##########
libminifi/include/utils/ResourceQueue.h:
##########
@@ -126,7 +137,10 @@ struct ResourceQueue<ResourceType>::make_shared_enabler :
public ResourceQueue<R
};
template<class ResourceType>
-auto ResourceQueue<ResourceType>::create(std::optional<size_t>
maximum_number_of_creatable_resources, std::shared_ptr<core::logging::Logger>
logger) {
- return
std::make_shared<make_shared_enabler>(maximum_number_of_creatable_resources,
std::move(logger));
+auto
ResourceQueue<ResourceType>::create(std::function<std::unique_ptr<ResourceType>()>
creator,
+ std::optional<size_t>
maximum_number_of_creatable_resources,
+
std::optional<std::function<void(ResourceType&)>> reset_fn,
+
std::shared_ptr<core::logging::Logger> logger) {
+ return std::make_shared<make_shared_enabler>(std::move(creator),
maximum_number_of_creatable_resources, std::move(reset_fn), std::move(logger));
Review Comment:
This is not a new change, but in `make_shared_enabler`, the forwarding
constructor is unnecessary complexity. The same can be achieved by adding this
in place of line 135-136:
```cpp
using ResourceQueue<ResourceType>::ResourceQueue;
```
##########
extensions/splunk/PutSplunkHTTP.cpp:
##########
@@ -40,32 +40,26 @@ void PutSplunkHTTP::initialize() {
setSupportedRelationships(relationships());
}
-void PutSplunkHTTP::onSchedule(const std::shared_ptr<core::ProcessContext>&
context, const std::shared_ptr<core::ProcessSessionFactory>& sessionFactory) {
- SplunkHECProcessor::onSchedule(context, sessionFactory);
- client_queue_ =
utils::ResourceQueue<extensions::curl::HTTPClient>::create(getMaxConcurrentTasks(),
logger_);
-}
-
namespace {
std::optional<std::string> getContentType(core::ProcessContext& context, const
core::FlowFile& flow_file) {
return context.getProperty(PutSplunkHTTP::ContentType) | utils::orElse
([&flow_file] {return flow_file.getAttribute("mime.type");});
}
-std::string getEndpoint(core::ProcessContext& context, const
gsl::not_null<std::shared_ptr<core::FlowFile>>& flow_file, curl::HTTPClient&
client) {
+std::string getEndpoint(core::ProcessContext& context, curl::HTTPClient&
client) {
std::stringstream endpoint;
endpoint << "/services/collector/raw";
std::vector<std::string> parameters;
- std::string prop_value;
- if (context.getProperty(PutSplunkHTTP::SourceType, prop_value, flow_file)) {
- parameters.push_back("sourcetype=" + client.escape(prop_value));
+ if (auto source_type = context.getProperty(PutSplunkHTTP::SourceType)) {
+ parameters.push_back("sourcetype=" + client.escape(*source_type));
}
- if (context.getProperty(PutSplunkHTTP::Source, prop_value, flow_file)) {
- parameters.push_back("source=" + client.escape(prop_value));
+ if (auto source = context.getProperty(PutSplunkHTTP::Source)) {
+ parameters.push_back("source=" + client.escape(*source));
}
- if (context.getProperty(PutSplunkHTTP::Host, prop_value, flow_file)) {
- parameters.push_back("host=" + client.escape(prop_value));
+ if (auto host = context.getProperty(PutSplunkHTTP::Host)) {
+ parameters.push_back("host=" + client.escape(*host));
}
- if (context.getProperty(PutSplunkHTTP::Index, prop_value, flow_file)) {
- parameters.push_back("index=" + client.escape(prop_value));
+ if (auto index = context.getProperty(PutSplunkHTTP::Index)) {
+ parameters.push_back("index=" + client.escape(*index));
Review Comment:
This change causes these properties to no longer support expression
language. Is this intentional?
##########
libminifi/test/unit/ResourceQueueTests.cpp:
##########
@@ -76,20 +81,39 @@ TEST_CASE("Resource limitation is not set to the resource
queue", "[utils::Resou
}
TEST_CASE("resource returns when it goes out of scope",
"[utils::ResourceQueue]") {
- auto queue = utils::ResourceQueue<int>::create(std::nullopt, nullptr);
+ using std::chrono::steady_clock;
+ auto queue = utils::ResourceQueue<steady_clock::time_point>::create([]{
return
std::make_unique<steady_clock::time_point>(steady_clock::time_point::min()); });
+ {
+ auto resource = queue->getResource();
+ CHECK(*resource == steady_clock::time_point::min());
+ *resource = steady_clock::now();
+ }
+ {
+ auto resource = queue->getResource();
+ CHECK(*resource != steady_clock::time_point::min());
+ }
+}
+
+TEST_CASE("resource resets when it goes out of scope",
"[utils::ResourceQueue]") {
+ using std::chrono::steady_clock;
+ auto queue = utils::ResourceQueue<steady_clock::time_point>::create([]{
return
std::make_unique<steady_clock::time_point>(steady_clock::time_point::min()); },
+
std::nullopt,
+
[](steady_clock::time_point& resource){ resource =
steady_clock::time_point::max();});
Review Comment:
A better way to test this would be to have the reset function assign the
same initial value as the create function, but change the value in the first
scope, and check that it's back to the default in the second.
This way you're testing the requirements instead of the 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
