MINIFICPP-550 - Implement RocksDB controller service and component st… by bakaid · Pull Request #605 · apache/nifi-minifi-cpp

bakaid · 2019-07-04T14:48:43Z

…ate storage

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 master)?
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?
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 travis-ci for build issues and submit an update to your PR as soon as possible.

bakaid · 2019-07-04T14:52:38Z

Known parts that are missing or at least should be discussed are:

strategy for deleting the state of the processors that are no longer used (how do we define this? on load, reload, restart?)
enforcing the type of the controller service supplied for state storage
- if we want to use KeyValueStoreServices as Processor properties we have to add a (accepts-descant-of or accepts-multiple-types kind of validator, both to minifi and to EFM
- as the controller service used for state storage is not a Processor Property, the same kind of validation would probably not work there

phrocker · 2019-08-29T17:09:17Z

@bakaid I think this is a pretty important piece of functionality especially as it relates to dealing with state more gracefully on windows (CWEL needs for example ) -- is this ready for review?

bakaid · 2019-08-30T08:48:47Z

@phrocker I think it's important as well - I raised the questions in my previous comment here, because I would like input on them, but I think someone would have to see the PR before they could be answered. So yes, by all means, I would appreciate if someone could review this.

phrocker

@bakaid You raise some good points. I think simplicity should reign. I'm in favor of defining this through the properties and not the config yml ( at least as a first step). We may get to that point but I think we should update the schema to get there.

As a result, we can define these via the minifi properties file. This will allow us to have modes for state removal. Reload, restart, and reference based are all reasonable. As a result I would not favore using KeyValueStoreServices as processor properties quite yet, but when that time comes our schema already supports a base class. Take for example
core::PropertyBuilder::createProperty("SSL Context Service")->withDescription("[...])->isRequired(false)->asType<minifi::controllers::SSLContextService>()->build());

I assume that is what you mean by accepts-descendant of?

The last point is, in my opinion, easier to answer via focusing on controller services as a service based implementations for the flow and the agent itself. When considering the latter, the lifetime could persist beyond the flow, but we certainly need to be aware of this and clean up if say the flow changes and we no longer have references to those items. We have cleanup for flow files and provenance, so this reference counting isn't unheard of in the agent. The flow is an easier case where stopping these components should result in cleanup.

phrocker · 2019-07-19T13:47:56Z

extensions/rocksdb-repos/controllers/RocksDbPersistableKeyValueStoreService.cpp

+void RocksDbPersistableKeyValueStoreService::notifyStop() {
+  AbstractAutoPersistingKeyValueStoreService::notifyStop();
+
+  db_valid_ = false;


Would love some defensiveness here to ensure rogue extensions don't cause a memory error.

arpadboda

Let's resurrect this as it's good stuff!

extensions/standard-processors/controllers/UnorderedMapKeyValueStoreService.h

szaszm

my two cents

szaszm · 2020-01-13T19:00:01Z

extensions/sftp/processors/ListSFTP.cpp

-    logger_->log_error("Failed to open Tracking Timestamps state file \"%s\"", tracking_timestamps_state_filename_.c_str());
-    return false;
-  }
+bool ListSFTP::updateFromTrackingTimestampsCache(const std::shared_ptr<core::ProcessContext>& context, const std::string& hostname, const std::string& username, const std::string& remote_path) {


Why do we use shared_ptr const&? If we can guarantee that the pointed-to object stays alive while we have a reference to it, then a regular observer pointer (plain ptr or tagged observer_ptr) states the purpose better.
This type says: "I'm a pointer to a mutable ProcessContext with shared ownership that can be reassigned by someone else to point to another mutable ProcessContext with shared ownership."

Agreed, we use it because onTrigger already gets it this way and it is easier to just pass that to these functions.

Are you open to changing it to core::ProcessContext& wherever the API allows that? It would simplify the API, stop saying the wrong thing (my original comment), not allow for null (except in arcane cases) and make future "de-shared_ptr-ification" refactorings easier.

I am not really open to in the context of this PR. This is preexisting code, and is used this way in many processors. If you want to do a round of these modifications to make easier further API changes, that makes sense, but should be a separate issue (and ticket).

extensions/standard-processors/controllers/UnorderedMapKeyValueStoreService.h

libminifi/include/controllers/keyvalue/PersistableKeyValueStoreService.h

szaszm · 2020-01-14T12:06:41Z

libminifi/include/core/ProcessContext.h

    return controller_service_provider_->getControllerServiceName(identifier);
  }

+  static constexpr char const* DefaultStateManagerProviderName = "defaultstatemanagerprovider";


This line reminds me of stereotypical enterprise codebases. We might have too much abstraction here.
default- ok
state- stateless is generally better, but ok
manager- meaningless. Only suggests doing something, so it should probably be a verb. Having a noun in place of a verb usually means trying to use a class where a function would be better.
provider- again, a class instead of a function. This has a meaning though.
name- ok

I'm not saying that your code is bad, just that this is a red flag that suggests a design mistake that may or may not be in your changes.

default- ok
state- stateless is generally better, but ok

This is a PR about creating a centralized state storage mechanism for processors that require state, instead of all of them implementing individual state files. "Stateless is generally better" is a mind-bogglingly meaningless comment here.

manager- meaningless. Only suggests doing something, so it should probably be a verb. Having a noun in place of a verb usually means trying to use a class where a function would be better.

StateManager comes from NiFi, and it is a perfectly meaningful name: it is an object that helps manage the state of a single CoreComponent. It cannot be a function as it contains some caching and validation logic that requires it in itself to be stateful.

provider- again, a class instead of a function. This has a meaning though.

CoreComponentStateManagerProvider needs to exist as interface, which is used by the ClassLoader and we access different implementations of it through that interface.

name- ok

I'm not saying that your code is bad, just that this is a red flag that suggests a design mistake that may or may not be in your changes.

I'm not saying your comment is useless, but it could be more useful if you tried to interpret the name in the context of the PR, instead of in itself.

Context is always important when reviewing code. Most code has a history even if we don't like it, so it may be useful to appreciate where it comes from and how we intend to get it where we want it to go.

Sorry for coming through as offensive. I lack a lot of contextual knowledge and at the time of writing the original comment, I knew even less.

I maintain my standpoint that there is most likely too much abstraction but now I understand that changing this would require redesign of significant parts of the project and it is harmful to do so without serious thought.

szaszm · 2020-01-14T12:41:00Z

libminifi/include/controllers/keyvalue/PersistableKeyValueStoreService.h

+namespace minifi {
+namespace controllers {
+
+class PersistableKeyValueStoreService : virtual public KeyValueStoreService, public AbstractCoreComponentStateManagerProvider {


I believe representing a combination of these capabilities via composition would be better design than representing them via inheritance.

PersistableKeyValueStoreService is just an interface extension of KeyValueStoreService, and forcefully wrapping a PersistableKeyValueStoreService into some class that provides the CoreComponentStateManagerProvider interface after loading the PersistableKeyValueStoreService with the classloader would make it impossible to implement and use a CoreComponentStateManagerProvider directly.
This way everything "just works": if you implement a PersistableKeyValueStoreService you automatically have a CoreComponentStateManagerProvider, but you can implement a CoreComponentStateManagerProvider directly.

szaszm · 2020-01-14T13:50:34Z

libminifi/src/controllers/keyvalue/AbstractCoreComponentStateManagerProvider.cpp

+    std::shared_ptr<AbstractCoreComponentStateManagerProvider> provider,
+    const std::string& id)
+    : provider_(std::move(provider))
+    , id_(id)


I prefer pass-by-value and move. I'd use it for id as well, not only provider.

It allows moving in values without extra overloads.

It's noexcept

It costs one extra move construction + destruction

libminifi/test/TestBase.cpp

libminifi/test/TestBase.h

libminifi/test/integration/IntegrationBase.h

szaszm · 2020-01-14T14:20:40Z

libminifi/test/keyvalue-tests/PersistableKeyValueStoreServiceTest.cpp

+  const char* key = "foobar";
+  const char* value = "234";
+  const char* new_value = "baz";
+  REQUIRE(true == controller->set(key, value));


Comparing a boolean expression to true is redundant.

Yep, but in tests I think it is better readable.

bakaid · 2020-03-11T02:18:17Z

@arpadboda @szaszm The PR is ready for review, with the following issues.

Blocking the PR:

When loading a new configuration through C2, notifyStop is not called on the ControllerServices, resulting in the RocksDB database staying open when the new controller service in the new configuration tries to reopen it, making the state retrieval fail and the processors think that they have no state. I am not sure how yet, but this could most likely be hotfixed without the whole memory leak/reload validation refactor. Will take a look next week.

Follow-up issues:

validation of descendant-of-* Properties: since in the new version of this PR the default injected CoreComponentStateManagerProvider is configurable through minifi.properties (whether it should be always persisting, and if not, what should be the auto persistence interval), the possibility of using your own controller service for this (defined in config.yml) becomes not that important for the time being. You can still do it, it will just lack a preemptive validation. Follow-up issue created: https://issues.apache.org/jira/browse/MINIFICPP-1173
cleaning up state storage: this is a hard question. I am not sure when we want to clean up state storage at all. For example, if a new configuration is loaded, we might want to clean the state of components that no longer exist in the flow, but this would mean that a single misconfiguration would make us loose our state (and that we can't properly roll back to an older configuration, because we have lost the state of processors no longer referenced). For the time being I think it is perfectly fine not to do any state cleanup: we don't have many processors using state, we don't have many instances of those processors that do use state, and states are small strings. We can handle the at maximum few hundred states stored in our DB (and this is the most extreme example I can imagine). If it really becomes an issue for someone, they can just delete the state directory/file. Follow-up issue created: https://issues.apache.org/jira/browse/MINIFICPP-1174
ControllerService notifyStop and destruction on shutdown: ControllerServices don't get destructed on shutdown (known shared_ptr cycle issue), but they also don't get a notifyStop, which Processors at least get. This is an issue, because this way the state won't be persisted on shutdown. I have worked this around by including an explicit persist in every Processors's notifyStop that uses state, but it should be fixed properly on the long run. Follow-up issue created: https://issues.apache.org/jira/browse/MINIFICPP-1175
ListSFTP migration: ListSFTP was released in 0.7.0, but I don't know about it being used in the wild. I have rewrote it for the new state handling, but didn't write any migration logic. If it is acceptable, I would prefer not to, and just make it drop the old state in the new release. I have created a blocking (for the next release) follow-up issue for this: https://issues.apache.org/jira/browse/MINIFICPP-1176
TailFile: TailFile is messed up. I have written state migration for it, both for the legacy and new single mode and multiple mode, but TailFile itself has issues, especially with multiple mode and rollover, it should really be rewritten from the grounds up sometime after we merged this PR. Follow-up issue created: https://issues.apache.org/jira/browse/MINIFICPP-1177
C2: we have to make the state storage queryable and per-component clearable through C2 for it to be really useful. I have done some preliminary work for this in this PR, but mostly for validating that it fits the architecture. Follow-up issue created: https://issues.apache.org/jira/browse/MINIFICPP-1178

All the processors that (to my knowledge) used state has been rewritten to use the new mechanism:

TailFile: tested manually, created automated migration tests
ListSFTP: tested with automated tests
QueryDatabaseTable: tested state migration and normal usage manually
ConsumeWindowsEventLog: tested state migration and normal usage manually

bakaid · 2020-03-11T02:27:52Z

libminifi/src/c2/C2Agent.cpp

      }
      enqueue_c2_response(std::move(response));
    }
+  } else if (resp.name == "corecomponentstate") {


This has only been tested manually. Once #743 is done and merged, I can probably write reasonable tests for it.

Added C2DescribeCoreComponentStateTest for this.

bakaid · 2020-03-11T02:28:22Z

libminifi/src/c2/C2Agent.cpp

        update_sink_->drainRepositories();
        C2Payload response(Operation::ACKNOWLEDGE, resp.ident, false, true);
        enqueue_c2_response(std::move(response));
+      } else if (resp.name == "corecomponentstate") {


This has only been written to make sure that it fits the architecture, it hasn't even been tested manually. Once #743 is done and merged, I can probably write reasonable tests for it.

Could you add a TODO: untested comment to the top of the block?

Let's revisit this after rebasing.

This is unfortunately still very much non-trivial to do (unlike testing DESCRIBE, which I do now), so I've added a TODO for it as suggested.

arpadboda

Mostly looks good, added some comments.
Didn't finish reviewing, further comments/questions are expected.

arpadboda · 2020-03-16T23:08:14Z

extensions/rocksdb-repos/controllers/RocksDbPersistableKeyValueStoreService.h

+
+  rocksdb::DB* db_;
+  rocksdb::WriteOptions default_write_options;
+  bool db_valid_;


Why do we need this? The pointer being null could indicate the db not being valid.

The documentation of rocksdb::open seems to fit in this aspect:
// Stores nullptr in *dbptr and returns a non-OK status on error.
(source: https://github.com/facebook/rocksdb/blob/master/include/rocksdb/db.h )

It was a remnant from the UnorderedMap version (which was implemented before this). Replaced along with using a unique_ptr.

arpadboda · 2020-03-16T23:08:34Z

extensions/rocksdb-repos/controllers/RocksDbPersistableKeyValueStoreService.h

+ protected:
+  std::string directory_;
+
+  rocksdb::DB* db_;


Wonder if it could be a unique ptr

None of our rocksdb stuff uses a unique_ptr, but it can, rewritten.

arpadboda · 2020-03-16T23:11:19Z

extensions/rocksdb-repos/controllers/RocksDbPersistableKeyValueStoreService.cpp

+    delete db_;
+  }
+  rocksdb::Options options;
+  options.create_if_missing = true;


We will need the buffer options to be set here the same way as we did for FF repo, otherwise the persistent storage will slowly generate the same 64 megabyte peaks we just got rid of.

Good point, this was written last summer, we didn't know this back then.

arpadboda · 2020-03-16T23:12:50Z

extensions/rocksdb-repos/controllers/RocksDbPersistableKeyValueStoreService.cpp

+}
+
+bool RocksDbPersistableKeyValueStoreService::set(const std::string& key, const std::string& value) {
+  if (!db_valid_) {


Wonder if we could utilize the nullcheck macro here (used id nanofi)

Replaced with unique_ptr, so no longer relevant.

arpadboda · 2020-03-16T23:20:54Z

extensions/sftp/processors/ListSFTP.cpp

-    logger_->log_error("Could not found json state file path in Tracking Entities state file \"%s\"", tracking_entities_state_filename_.c_str());
+  std::unordered_map<std::string, std::string> state_map;
+  if (!state_manager_->get(state_map)) {
+    logger_->log_debug("Failed to get state from StateManager");


Just debug?

Sometimes it's ok, but I wonder if it should be at least info.

If the processor has no stored state yet, state_manager_->get will return with false.
It could be modified to return an empty map, but I think there is value in having a distinction between "we have already stored a state, we just have nothing to store" and "we have no stored state yet".

That's completely reasonable, I put my focus on the error case: for eg. persistency is corrupted/deleted on the disk, so processors unexpectedly start from a clean state. Which worth at least an info level log message in my opinion.
Yes, I also expect the state manager to log something in such scenario, although loading persistency shouldn't happen too frequently (onschedule phase), so I don't mind being quite verbose here.

If we can make a distinction of such cases AND (if the state storage is present, but corrupted, OR the state storage is not present, but expected), then I'd like to log a warning, not an info.
The current API doesn't seem to allow for such distinction and I'm not sure whether modifying it is worth it or even possible.

arpadboda · 2020-03-16T23:25:34Z

extensions/windows-event-log/Bookmark.cpp

    logger_->log_error("No '!' in bookmarXml '%ls'", bookmarkXml.c_str());
    bookmarkXml.clear();
-    return createEmptyBookmarkXmlFile();
+	return false;


This seems to be misindented.

Thanks, since these are Windows-only, I edited them in VS when migrating them, and well... this is the result.

If you find a simple way to disable automatic reformatting in VS, I'm interested as well. I sometimes have to recreate my windows changes on my linux editor to avoid huge diffs due to automatic reformatting.
There are a handful of options, but no obvious (to me) way to disable messing with unchanged code.

arpadboda · 2020-03-16T23:26:20Z

extensions/windows-event-log/Bookmark.cpp

+				utils::file::FileUtils::concat_path(bookmarkRootDir, "uuid"), uuid), "Bookmark.txt");
+
+    std::wstring bookmarkXml;
+		if (getBookmarkXmlFromFile(bookmarkXml)) {


Some spaces/tabs issue here and above in this functions as well

extensions/standard-processors/controllers/UnorderedMapKeyValueStoreService.h

arpadboda · 2020-03-16T23:47:05Z

extensions/standard-processors/controllers/UnorderedMapPersistableKeyValueStoreService.h

+namespace minifi {
+namespace controllers {
+
+class UnorderedMapPersistableKeyValueStoreService : public AbstractAutoPersistingKeyValueStoreService,


What's the motivation behind these class?

The implementation looks good, but I'm not sure if there is any value in having a maintaining a 3rd store service.
I wonder if we could utilise a very tiny and simple 3rd party kv storage for this purpose instead of having an own implementation.

The non-serialized makes perfect sense for "no footprint" usage, it can be configured together with volatile repos.
The rocksdb one allignes perfectly with the rocksdb repos.

This one stands somewhere in the middle, to ensure persistency in case of not having rocksdb, although I feel that we introduce a something we will have to support for long and it doesn't really add value.

The motivation is twofold: to provide an implementation without any third party requirements, and to provide a serialized format which is human-readable and easily editable.

As for the first motivation: we don't want to make it a hard requirement to have rocksdb to use stateful processors, because that would hinder low-footprint usages, nor do I want to make any other third party a dependency for this: it would, again, introduce an unwanted requirement for low-footprint usages and we would have to find one that supports all target platforms, and maintain it, which is not a negligible maintenance burden.

For the second one: so far users could easily clear a state by deleting the state file or directory. With the rocksdb state storage this is no longer a trivial task. The C2 methods introduced here attempt to solve this problem, but until they are finalized (or if someone does not want to use, or can't use C2), they aren't really useful.
A solution for this is using this state storage method: its serialized format is trivial and editable in text format without any third party tools, making it easy to clear (or even modify) the state of processors.

Finally for the support argument: this is a trivial, and, if you take a look at PersistableKeyValueStoreServiceTest.cpp, well-tested implementation, that I don't think would introduce a significant maintenance burden going forward.

szaszm · 2020-04-15T16:15:00Z

libminifi/test/keyvalue-tests/CMakeLists.txt

+add_test(NAME UnorderedMapPersistableKeyValueStoreServiceTest COMMAND PersistableKeyValueStoreServiceTest --config-yaml "${TEST_RESOURCES}/UnorderedMapPersistableKeyValueStoreServiceTest.yml")
+if (NOT DISABLE_ROCKSDB)
+    add_test(NAME RocksdDbPersistableKeyValueStoreServiceTest COMMAND PersistableKeyValueStoreServiceTest --config-yaml "${TEST_RESOURCES}/RocksDbPersistableKeyValueStoreServiceTest.yml")
+endif()


missing line break at the end of the file

szaszm · 2020-04-15T16:24:09Z

libminifi/test/TestBase.cpp

+  if (state_dir == nullptr) {
+    char state_dir_name_template[] = "/tmp/teststate.XXXXXX";
+    state_dir_ = utils::file::FileUtils::create_temp_directory(state_dir_name_template);
+  } else {
+    state_dir_ = state_dir;
+  }
+  state_manager_provider_ = core::ProcessContext::getOrCreateDefaultStateManagerProvider(controller_services_provider_, configuration_, state_dir_.c_str());


We should move the state dir to /var/tmp to avoid breaking the tests on tmpfs when using rocksdb storage, as described in MINIFICPP-1188.

szaszm · 2020-04-15T16:24:56Z

libminifi/test/integration/IntegrationBase.h

+  auto controller_service_provider = yaml_ptr->getControllerServiceProvider();
+  char state_dir_name_template[] = "/tmp/integrationstate.XXXXXX";
+  state_dir = utils::file::FileUtils::create_temp_directory(state_dir_name_template);
+  core::ProcessContext::getOrCreateDefaultStateManagerProvider(controller_service_provider, configuration, state_dir.c_str());


We should move the state dir to /var/tmp to avoid breaking the tests on tmpfs when using rocksdb storage, as described in MINIFICPP-1188.

szaszm · 2020-04-16T15:29:50Z

extensions/sftp/processors/ListSFTP.cpp

-    logger_->log_error("Failed to open Tracking Timestamps state file \"%s\"", tracking_timestamps_state_filename_.c_str());
-    return false;
-  }
+bool ListSFTP::updateFromTrackingTimestampsCache(const std::shared_ptr<core::ProcessContext>& context, const std::string& hostname, const std::string& username, const std::string& remote_path) {


Are you open to changing it to core::ProcessContext& wherever the API allows that? It would simplify the API, stop saying the wrong thing (my original comment), not allow for null (except in arcane cases) and make future "de-shared_ptr-ification" refactorings easier.

szaszm · 2020-04-16T15:52:43Z

extensions/sftp/processors/ListSFTP.cpp

+    try {
+      state_listing_timestamp = stoull(state_map.at("listing.timestamp"));
+    } catch (...) {
+      return false;
    }
-    std::string key = line.substr(0, separator_pos);
-    std::string value = line.substr(separator_pos + 1);
-    if (key == "hostname") {
-      state_hostname = std::move(value);
-    } else if (key == "username") {
-      state_username = std::move(value);
-    } else if (key == "remote_path") {
-      state_remote_path = std::move(value);
-    } else if (key == "listing.timestamp") {
-      try {
-        state_listing_timestamp = stoull(value);
-      } catch (...) {
-        logger_->log_error("listing.timestamp is not an uint64 in Tracking Timestamps state file \"%s\"", tracking_timestamps_state_filename_.c_str());
-        return false;
-      }
-    } else if (key == "processed.timestamp") {
-      try {
-        state_processed_timestamp = stoull(value);
-      } catch (...) {
-        logger_->log_error("processed.timestamp is not an uint64 in Tracking Timestamps state file \"%s\"", tracking_timestamps_state_filename_.c_str());
-        return false;
+    try {
+      state_processed_timestamp = stoull(state_map.at("processed.timestamp"));
+    } catch (...) {
+      return false;
+    }


The inner catch-alls are redundant, since the outer one does the same: return false.
I'd prefer to have the log messages restored, which may make nested try blocks necessary again.

Added individual logging.

szaszm · 2020-04-19T02:13:52Z

extensions/standard-processors/tests/unit/TailFileTests.cpp

  std::mt19937 gen(std::random_device { }());
  std::generate_n(line1.begin(), 4095, [&]() -> char {
-    return 32 + gen() % (127 - 32);
+  return 32 + gen() % (127 - 32);


The old indentation looks correct to me.

std::uniform_int_distribution{ 32, 126 } would be a more readable way of generating these characters, if you're interested in changing this piece of old code.

I am not really.

szaszm · 2020-04-19T02:32:41Z

libminifi/src/core/ConfigurableComponent.cpp

+  std::lock_guard<std::mutex> lock(configuration_mutex_);
+
+  for (auto item : properties) {
+    properties_[item.getName()] = item;


We can avoid both copies of each property.

for (auto& item: properties) { const auto name = item.getName(); properties_[name] = std::move(item); }

This function is a modified version of ConfigurableComponent::setSupportedProperties. Yes, the copies can be avoided both here and there, but it is completely inconsequential, as this happens only once per flow initialization.

szaszm · 2020-04-20T08:57:03Z

extensions/standard-processors/processors/TailFile.cpp

-  for (file.getline(buf, BUFFER_SIZE); file.good(); file.getline(buf, BUFFER_SIZE)) {
-    parseStateFileLine(buf);
+
+  /* We could not get the state form the StateManager, try to migrate the old state file if it exists */


typo: s/form/from/

szaszm · 2020-04-20T14:55:08Z

libminifi/src/controllers/keyvalue/AbstractCoreComponentStateManagerProvider.cpp

+AbstractCoreComponentStateManagerProvider::~AbstractCoreComponentStateManagerProvider() {
+}


It's possible to use = default; in .cpp files as well.

szaszm · 2020-04-20T16:33:32Z

libminifi/src/c2/C2Agent.cpp

        update_sink_->drainRepositories();
        C2Payload response(Operation::ACKNOWLEDGE, resp.ident, false, true);
        enqueue_c2_response(std::move(response));
+      } else if (resp.name == "corecomponentstate") {


Could you add a TODO: untested comment to the top of the block?

…ate storage

bakaid · 2020-04-26T15:40:12Z

@arpadboda @szaszm Rebased to latest master (including C2 refactor), added test for C2 DESCRIBE corecomponentstate, addressed review comments.
I have tested state handling on shutdown and C2 configuration reload with TailFile, it seems to work fine.
I am going to continue with final verification of functionality/state migration of stateful processors on different platforms, if those work fine I think this should be ready to merge.

bakaid · 2020-04-27T16:16:53Z

I am done with my final verification, the following has been tested successfully:

Using the new state storage for:
- TailFile
- ListSFTP
- CWEL
- QDT
State migration from old state file/directory for:
- TailFile
- CWEL
- QDT
State-related minifi properties:
- setting nifi.state.management.provider.local.always.persist to true persists every state change to disk immediately
- setting nifi.state.management.provider.local.auto.persistence.interval to 5 sec makes state changes persist to the disk more quickly
- using a custom configured RocksDbPersistableKeyValueStoreService through nifi.state.management.provider.local works properly
With both a rocksdb-enabled (default injected state storage RocksDbPersistableKeyValueStoreService) and rocksdb-disabled (default injected state storage UnorderedMapPersistableKeyValueStoreService) the following:
- default injected storage works properly
- state is persisted on normal shutdown
- state is persisted on C2 configuration reload

arpadboda · 2020-04-27T19:47:49Z

Built and tested, seems to work based on my tests as well, so LGTM, merging this.
Follow-ups may naturally pop up.

bakaid force-pushed the MINIFICPP-550 branch 2 times, most recently from cee2125 to 1600e71 Compare July 5, 2019 08:51

phrocker reviewed Sep 9, 2019

View reviewed changes

arpadboda reviewed Oct 23, 2019

View reviewed changes

extensions/standard-processors/controllers/UnorderedMapKeyValueStoreService.h Show resolved Hide resolved

szaszm requested changes Jan 14, 2020

View reviewed changes

bakaid force-pushed the MINIFICPP-550 branch from ed99430 to 4b51ded Compare March 9, 2020 15:17

bakaid marked this pull request as ready for review March 11, 2020 02:18

bakaid commented Mar 11, 2020

View reviewed changes

arpadboda reviewed Mar 16, 2020

View reviewed changes

szaszm requested changes Apr 20, 2020

View reviewed changes

bakaid force-pushed the MINIFICPP-550 branch from 22f5c38 to d6022c7 Compare April 24, 2020 12:16

bakaid and others added 14 commits April 25, 2020 19:19

MINIFICPP-550 - Implement RocksDB controller service and component st…

e646a10

…ate storage

wip

472038c

win build fix

163fec0

win test fix

838cfe3

TailFile fixes

12ca2b4

QDT state WiP

8043792

fixes

bbf594c

refactor

e423dd9

CWEL wip

cb9d564

C2 wip

f606ab8

review fixes

2266a2e

wip

a029aae

fixes

538920a

fix

a79c4be

bakaid added 8 commits April 25, 2020 19:21

Fix reload and stop issues

e211719

Review fixes

9ae9608

Review fixes

edecc62

Review fixes

96dc940

Review fixes

8aca62b

Fixes

82e3bc5

C2DescribeCoreComponentStateTest

380a1a8

Review fix

e350d3c

bakaid force-pushed the MINIFICPP-550 branch from d6022c7 to e350d3c Compare April 26, 2020 15:12

Review fix

06799f4

RocksDbPersistableKeyValueStoreService register fix

42825e0

arpadboda approved these changes Apr 27, 2020

View reviewed changes

arpadboda closed this in fb2bf6a Apr 27, 2020

		AbstractCoreComponentStateManagerProvider::~AbstractCoreComponentStateManagerProvider() {
		}

Comments

Conversation

bakaid commented Jul 4, 2019

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

bakaid commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phrocker commented Aug 29, 2019

Uh oh!

bakaid commented Aug 30, 2019

Uh oh!

phrocker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arpadboda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szaszm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bakaid Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bakaid Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bakaid commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arpadboda left a comment

Choose a reason for hiding this comment

bakaid commented Jul 4, 2019 •

edited

Loading

phrocker left a comment •

edited

Loading

bakaid Mar 9, 2020 •

edited

Loading

bakaid Mar 10, 2020 •

edited

Loading

bakaid commented Mar 11, 2020 •

edited

Loading