[GitHub] [nifi-minifi-cpp] bakaid commented on issue #605: MINIFICPP-550 - Implement RocksDB controller service and component st…

[GitBox <gi...@apache.org>]

bakaid commented on issue #605: MINIFICPP-550 - Implement RocksDB controller service and component st…
URL: https://github.com/apache/nifi-minifi-cpp/pull/605#issuecomment-597409388
 
 
   @aboda @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
   
   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

----------------------------------------------------------------
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