ISSUE-133: Make Hydroponics background processing multi child based

[giancarlobi]

A lot of hydroponics improvements. See #133

[DiegoPino]

@giancarlobi looking great. I added some minor comments on cleanup and may give all another pass to make sure we remove comments/etc.

Also some minor notes on configurations, like making the INTs instead of Strings for time based values

[DiegoPino]

What if $queuerunner_pid is not >0? should we not stop processing and return immediately?

[giancarlobi]

Sorry, not clear to me what you mean.

[DiegoPino]

We may want to have a comment somewhere like here https://github.com/esmero/strawberryfield/pull/138/files#diff-f87fbb18e8f039828bd65f2204e85b8a323c7f253f13ea2f5109174060aa3a4eR388 (every time we fetch the PID) explaining that if its negative means it was started? basically explain why you use negatives for tracking PIDS.

[giancarlobi]

Added a more verbose checkRunning result, see if what you mean or not, please

[DiegoPino]

This message and the way we return messages here is something I'm not totally convinced of
1.- Messages are not translated.
2.- We depend on a message to define if stopped worked? DO we need a also a status returned? We could use maybe instead a StdClass Object to make sending/returning of data easier like we do for the actual workers?

[giancarlobi]

@DiegoPino As for checkRunning, stop function can return an array ['staus'] (TRUE all ok, FALSE for error) and ['message'] with text about result, what do you think?

[giancarlobi]

@DiegoPino Thanks, I'll parse everything and make changes as for your notes. Not sure if today, probably tomorrow. Anyway we need a full check before merging, obviously.

[DiegoPino]

@giancarlobi I see all your changes, this is great!

[giancarlobi]

@giancarlobi I see all your changes, this is great!

@DiegoPino I had some free relaxed time and started to make changes. Some left to solve, I hope later or tomorrow to complete all ... I hope

[DiegoPino]

@giancarlobi that works! Just for future compatibility its suggested (sorry, its only good practice) to use Drupal\Core\State\StateInterface; instead of use Drupal\Core\State\State; for the use statement and the argument annotations and casting in the functions. That way, in case someone needs to override the container injection with a different class that implements the same interface no code change is needed, just the service. yml. You will notice that we do that for almost everything except for the queue factory, since it extends (silly) a too generic interface that has really no methods at all.

[giancarlobi]

@giancarlobi that works! Just for future compatibility its suggested (sorry, its only good practice) to use Drupal\Core\State\StateInterface; instead of use Drupal\Core\State\State; for the use statement and the argument annotations and casting in the functions. That way, in case someone needs to override the container injection with a different class that implements the same interface no code change is needed, just the service. yml. You will notice that we do that for almost everything except for the queue factory, since it extends (silly) a too generic interface that has really no methods at all.

@DiegoPino i.e. as for \Psr\Log\LoggerInterface ? Thanks for note, I'll try to follow guide lines. Also today I learned something new, friend

[DiegoPino]

yes! True.

[DiegoPino]

wonderful!

[DiegoPino]

@giancarlobi first pass on code/sanity looks good! I need to now test with some data/actions and watch logs. What would you suggest is a good way of making sure all is working correctly? What is the the config equivalent of the old method (the simple one by one)? So I can compare? No rush. I will also do some small Drupal Coding Standard changes afterwards (probably a separate branch) so I do not interrupt your work here because I know you are busy. Basically I will create a RC2 copy branch and merge this against that one with each commit (not a squash) and apply the Coding standard changes (small really) to that one. Let me know if that works for you @giancarlobi. Good work!!

[giancarlobi]

Good morning @DiegoPino

@giancarlobi first pass on code/sanity looks good! I need to now test with some data/actions and watch logs. What would you suggest is a good way of making sure all is working correctly? What is the the config equivalent of the old method (the simple one by one)? So I can compare? No rush.

The config as the old method is using "A single process for all queues" in setting admin/config/archipelago/hydroponics

I will also do some small Drupal Coding Standard changes afterwards (probably a separate branch) so I do not interrupt your work here because I know you are busy. Basically I will create a RC2 copy branch and merge this against that one with each commit (not a squash) and apply the Coding standard changes (small really) to that one. Let me know if that works for you @giancarlobi. Good work!!

Perfect, thanks !!!

[DiegoPino]

@giancarlobi merging into a local branch ISSUE-133 to apply tiny Drupal coding standards changes. Will preserve the full commit log! This should be "finally" ready from my part by tomorrow. Thank you again for all your patience friend!

[giancarlobi]

@giancarlobi merging into a local branch ISSUE-133 to apply tiny Drupal coding standards changes. Will preserve the full commit log! This should be "finally" ready from my part by tomorrow. Thank you again for all your patience friend!

@DiegoPino don't worry, I'm happy with this. And thanks for your great work.

[DiegoPino]

@giancarlobi good morning there! (like 4AM I know).

I'm testing and debugging your code right now and I very much like it! Its very efficient and verbose!! Sorry for the delay on testing but there are too many things on my plate and I really wanted to test this in detail since its core to Archipelago

Right now I'm testing the Mono Command (HydroponicsDrushCommands)

One thing I noticed is that we do not have the security timer anymore. Means it will run into it has no more queue items. I feels its the right thing but thinking of a special use case (Multisite on a smaller server) I would like to add it back as an option (checkbox)
Do you think it is something I can do without breaking anything? Like just adding this back? I feel its not that simple right?

$security_time = 720.0;
$securitytimer = $loop->addTimer(720.0, function ($timer) use ($loop, $timer_ping, $idle_timer, &$done) {
      // Finish all if 720 seconds are reached
      \Drupal::logger('hydroponics')->info("720 seconds passed closing Hydroponics Service");
      $loop->cancelTimer($timer_ping);
      foreach($done as $queue_timer) {
        $loop->cancelTimer($queue_timer);
      }
      \Drupal::state()->set('hydroponics.queurunner_last_pid', 0);
      $loop->cancelTimer($idle_timer);
      $loop->stop();
      }
    );

Or, I should i add an extra time check inside

$idle_timer = $loop->addPeriodicTimer($idle_timer_time, function ($timer) use ($loop, $timer_ping, &$done, &$idle) ?

I may need your wise advice on this.
Thanks a lot!

[giancarlobi]

@DiegoPino Sorry for my silence into Archipelago community, here really busy days, I hope to free me to the end of this week. Don't worry about your delay on this, I know you are really really multi-tasking !!!
About security timer, at first look I think is better add a check into idle_timer than add an extra timer (we have to clear it before close and so on).
We can simply count how many times idle_timer loop is executed and (if enabled into config) close loop when N is reached. More or less time will be N * 60s with current hard coded settings.
Whant do you think?
Take care friend

[DiegoPino]

Hi Friend! Yes i love that idea. Will check the code and pass through your eyes! No worries about silence, you are with us the whole time! El El mié, 24 de feb. de 2021 a la(s) 08:09, Giancarlo < notifications@github.com> escribió:

@DiegoPino <https://github.com/DiegoPino> Sorry for my silence into Archipelago community, here really busy days, I hope to free me to the end of this week. Don't worry about your delay on this, I know you are really really multi-tasking !!! About security timer, at first look I think is better add a check into idle_timer than add an extra timer (we have to clear it before close and so on). We can simply count how many times idle_timer loop is executed and (if enabled into config) close loop when N is reached. More or less time will be N * 60s with current hard coded settings. Whant do you think? Take care friend — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#138 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABU7ZZ2NKM54NJZ6PRMYUZ3TAT27HANCNFSM4WCRS5FQ> .

-- Diego Pino Navarro Digital Repositories Developer Metropolitan New York Library Council (METRO)