fgerlits commented on code in PR #2157:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2157#discussion_r3087081839
##########
extensions/sql/processors/QueryDatabaseTable.cpp:
##########
@@ -155,11 +150,13 @@ bool
QueryDatabaseTable::loadMaxValuesFromStoredState(const std::unordered_map<s
void QueryDatabaseTable::initializeMaxValues(core::ProcessContext &context) {
max_values_.clear();
std::unordered_map<std::string, std::string> stored_state;
- if (!state_manager_->get(stored_state)) {
+
+ auto state_manager = context.createStateManager();
+ if (!state_manager->get(stored_state)) {
Review Comment:
also here:
```suggestion
if (!state_manager || !state_manager->get(stored_state)) {
```
##########
libminifi/src/core/ProcessContextImpl.cpp:
##########
@@ -166,10 +166,17 @@ bool ProcessContextImpl::isRunning() const {
return getProcessor().isRunning();
}
-StateManager* ProcessContextImpl::getStateManager() {
+std::unique_ptr<StateManager> ProcessContextImpl::createStateManager() {
if (state_storage_ == nullptr) { return nullptr; }
- if (!state_manager_) { state_manager_ =
state_storage_->getStateManager(processor_); }
- return state_manager_.get();
+ return state_storage_->createStateManager(processor_);
+}
+
+StateManager* ProcessContextImpl::getStateManager() {
+ return session_state_manager_.get();
+}
Review Comment:
Maybe rename this to `getSessionStateManager()` to make it clearer that this
function and `setSessionStateManager` are a getter-setter pair?
##########
extensions/aws/processors/ListS3.cpp:
##########
@@ -157,7 +151,15 @@ void ListS3::onTrigger(core::ProcessContext& context,
core::ProcessSession& sess
return;
}
- auto stored_listing_state = state_manager_->getCurrentState();
+ auto* state_manager = context.getStateManager();
Review Comment:
Should this be `createStateManager`?
I have to admit I don't really understand when `get[Session]StateManager`
and when `createStateManager` should be used; can you add an explanation
somewhere, please?
##########
extensions/grafana-loki/PushGrafanaLoki.cpp:
##########
@@ -167,6 +166,7 @@ void PushGrafanaLoki::processBatch(const
std::vector<std::shared_ptr<core::FlowF
}
void PushGrafanaLoki::onTrigger(core::ProcessContext& context,
core::ProcessSession& session) {
+ log_batch_.setStateManager(context.getStateManager());
Review Comment:
should we check here if the state manager is non-null? we do it at most
other places
##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -421,11 +416,12 @@ void ListSFTP::listByTrackingTimestamps(
const std::string& username,
const std::string& remote_path,
std::vector<Child> files) {
+ auto* state_manager = context.getStateManager();
Review Comment:
here, too, I would check if `state_manager` is non-null
##########
extensions/systemd/tests/ConsumeJournaldTest.cpp:
##########
@@ -164,8 +160,11 @@ TEST_CASE("ConsumeJournald", "[consumejournald]") {
minifi::core::ProcessorMetadata{utils::Identifier{}, "ConsumeJournald",
core::logging::LoggerFactory<ConsumeJournald>::getLogger()},
std::move(libwrapper)), "ConsumeJournald");
REQUIRE(consume_journald->setProperty(ConsumeJournald::TimestampFormat.name,
"ISO8601"));
- const auto get_cursor_position = [&consume_journald]() -> std::string {
- return
ConsumeJournaldTestAccessor::get_state_manager_(consume_journald.get())->get()->at("cursor");
+ const auto get_cursor_position = [&plan, &consume_journald]() -> std::string
{
+ auto sm =
plan->getProcessContextForProcessor(consume_journald)->createStateManager();
+ std::unordered_map<std::string, std::string> state;
+ sm->get(state);
+ return state.at("cursor");
Review Comment:
Nitpicking, but why did this change from `->get()-at("cursor")` to this?
Also, I wouldn't mind a few `REQUIRE`s to check for null, but since they
weren't there before, I don't insist.
##########
extensions/standard-processors/processors/TailFile.h:
##########
@@ -240,9 +240,9 @@ class TailFile : public core::ProcessorImpl {
void onTrigger(core::ProcessContext& context, core::ProcessSession& session)
override;
void initialize() override;
- bool recoverState(const core::ProcessContext& context);
+ bool recoverState(core::StateManager* state_manager, const
core::ProcessContext& context);
void logState();
- bool storeState() const;
+ bool storeState(core::StateManager* state_manager) const;
Review Comment:
These two functions (plus the two private functions below) which now take a
`StateManager*` argument assume that it is non-null; could they take a
`StateManager&` argument instead?
--
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]
