nifi-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #605: MINIFICPP-550 - Implement RocksDB controller service and component st…
Date Mon, 09 Mar 2020 15:10:29 GMT
bakaid commented on a change in pull request #605: MINIFICPP-550 - Implement RocksDB controller
service and component st…
URL: https://github.com/apache/nifi-minifi-cpp/pull/605#discussion_r389753888
 
 

 ##########
 File path: libminifi/include/core/ProcessContext.h
 ##########
 @@ -193,6 +216,61 @@ class ProcessContext : public controller::ControllerServiceLookup, public
core::
     return controller_service_provider_->getControllerServiceName(identifier);
   }
 
+  static constexpr char const* DefaultStateManagerProviderName = "defaultstatemanagerprovider";
 
 Review comment:
   > 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.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message