MINIFICPP-1288 - FlowController start/load refactor

[hunyadi-dev]

It is easiest to review this PR on a per-commit basis.

The FlowController methods: start, stop, load, unload (and maybe even waitUnload), were strongly coupled and were controlled through FlowController states that made it difficult to argue about what goes on in each of them.

load had two versions with only minor differences, one reload (with a few extra steps) and one non-reload. The version that did the reload was only called during applyConfiguration, so I moved the extra behaviour required there (and renamed the related functions accordingly to prove there were no other instances of calling reload).

load was also changing the state of initialized_ read by only itself and start, but it is easy to argue that load is never called twice and start is never called without a preceeding load, proving this variable clutter.

[arpadboda]

Most looks good, added some comments.

[szaszm]

Since the requirement of "one has to call load before start" is not documented anywhere, I would restore the initialized_ variable to prevent invalid usage of FlowController.
I fell into this trap of only calling stop/start instead of stop,unload/load,start while working on #875

[szaszm]

Consider using gsl::make_not_null and relying on implicit conversion to base class pointer.

Suggested change

	timer_scheduler_ = std::make_shared<TimerDrivenSchedulingAgent>(NonNullControllerServiceProviderPtr(this), provenance_repo_, flow_file_repo_, content_repo_, configuration_, thread_pool_);
	event_scheduler_ = std::make_shared<EventDrivenSchedulingAgent>(NonNullControllerServiceProviderPtr(this), provenance_repo_, flow_file_repo_, content_repo_, configuration_, thread_pool_);
	cron_scheduler_ = std::make_shared<CronDrivenSchedulingAgent>(NonNullControllerServiceProviderPtr(this), provenance_repo_, flow_file_repo_, content_repo_, configuration_, thread_pool_);
	timer_scheduler_ = std::make_shared<TimerDrivenSchedulingAgent>(gsl::make_not_null(this), provenance_repo_, flow_file_repo_, content_repo_, configuration_, thread_pool_);
	event_scheduler_ = std::make_shared<EventDrivenSchedulingAgent>(gsl::make_not_null(this), provenance_repo_, flow_file_repo_, content_repo_, configuration_, thread_pool_);
	cron_scheduler_ = std::make_shared<CronDrivenSchedulingAgent>(gsl::make_not_null(this), provenance_repo_, flow_file_repo_, content_repo_, configuration_, thread_pool_);

[hunyadi-dev]

What is the advantake of using make_not_null? It does nothing, does it?

Update:
I see what you mean now.

[hunyadi-dev]

Since the requirement of "one has to call load before start" is not documented anywhere, I would restore the initialized_ variable to prevent invalid usage of FlowController.
I fell into this trap of only calling stop/start instead of stop,unload/load,start while working on #875

I see what you mean, but I disagree. I would definitely avoid adding states like this to an object. I found it already quite a task to decipher in what state can we reach each of the member functions. How about either of these options:

1. I can make the FlowController a state machine with well defined transitions between states
2. I can add checks to the start method that ensure that the members set in load are indeed set.

[szaszm]

I see what you mean, but I disagree. I would definitely avoid adding states like this to an object. I found it already quite a task to decipher in what state can we reach each of the member functions. How about either of these options:
1. I can make the FlowController a state machine with well defined transitions between states
2. I can add checks to the start method that ensure that the members set in load are indeed set.

I don't really understand what you mean by making FlowController a state machine or checking members, but in either case, if you can make it hard or impossible to use FlowController incorrectly, that would be a big plus in my opinion.