Added sys:cron:schedule command #257

Merged
merged 9 commits into from Dec 31, 2016

Conversation

Projects
None yet
3 participants
@hostep
Contributor

hostep commented Dec 5, 2016

Magerun pull-request check-list:

  • Pull request against develop branch (if not, just close and create a new one against it)
  • README.md reflects changes (if any)

Sometimes we want to run a cronjob, but not keep our ssh session or terminal open while running sys:cron:run. This PR adds a sys:cron:schedule command which schedules a certain job in the cron schedule to be executed the next time the cron is being run.
Was inspired by the excellent Aoe_Scheduler module for M1.

Possible discussion points:

  1. The name schedule might be confusing, people might expect they can provide a timestamp on which time they want to schedule a job. Is this wanted? We could also name the command schedule-now or something like that?

  2. I copied a lot of code from the RunCommand class. This should definitely be refactored away. Should we put shared code in the AbstractCronCommand class, or someplace else?

  3. There is something wrong with the timezone of the times in the RunCommand class. I haven't fixed this yet over there, but this new class should work correctly. I took a look at how M2 itself does this and based my code on that.

If there are any remarks, let me know!

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 5, 2016

Current coverage is 51.09% (diff: 16.36%)

Merging #257 into develop will decrease coverage by 0.22%

@@            develop       #257   diff @@
==========================================
  Files           195        196     +1   
  Lines          8609       8664    +55   
  Methods         865        869     +4   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4418       4427     +9   
- Misses         4191       4237    +46   
  Partials          0          0          

Powered by Codecov. Last update 9ef43c7...b8f232f

Current coverage is 51.09% (diff: 16.36%)

Merging #257 into develop will decrease coverage by 0.22%

@@            develop       #257   diff @@
==========================================
  Files           195        196     +1   
  Lines          8609       8664    +55   
  Methods         865        869     +4   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4418       4427     +9   
- Misses         4191       4237    +46   
  Partials          0          0          

Powered by Codecov. Last update 9ef43c7...b8f232f

@ktomk

This comment has been minimized.

Show comment
Hide comment
@ktomk

ktomk Dec 5, 2016

Contributor

Possible discussion points:

  1. The name schedule might be confusing, people might expect they can provide a timestamp on which time they want to schedule a job. Is this wanted? We could also name the command schedule-now or something like that?
    AFAIK we can't do that as in the end Magento decides when to execute it and it depends on how many there are. There is a timestamp so it can be set, but remeber it must not be executed at that time. It's something line "not before" that timestamp. Maybe defer this for a moment.

For the wording, please also take a look with Magerun 1 where - IIRC - a cron schedule command exists. In any case this should be backwards compatible. If this does not match, let's defer the details. But if you can, take a look.

  1. I copied a lot of code from the RunCommand class. This should definitely be refactored away. Should we put shared code in the AbstractCronCommand class, or someplace else?

If you can refactor it into a model class, I'd say it's preferable over an abstract base class as we should favor aggregation over inheritance. You can place such a model class next to those command classes (in the same directory). This might take more time than pushing members down to a new abstract-base-class. You're open to do it the way you prefer (or what currently works best for you), so no hard rules here. Historically, abstract classes were preferred, for me personally that's not my style but just find your way that is practically for you right now.

  1. There is something wrong with the timezone of the times in the RunCommand class. I haven't fixed this yet over there, but this new class should work correctly. I took a look at how M2 itself does this and based my code on that.
    I have no decision on this yet. I've seen the last ~3-5 days that Magento uses strftime which is locale dependent so I can imagine (but have not checked as only just seen) that locale might incorporate the timezone for the timestamp (sorry no definite check on it, references welcome). If that (locale and timezone dependent timestamp) is the case it would violate the best practice to use UTC based timestamps (not timezone dependent ones). So bringing inconsistencies in the mix (locale is not thread-safe) this might require the work-around you linked.

From this quick glance over the last days (was looking in there because of some Magerun 1 cron command), I'd say the use of strftime within Magento core definitely is a smell and I wondered why Magento core makes use of it for timestamps stored into the database system. Really.

For your question: I would prefer to not duplicate any work-arounds (if there are any) but instead do as documented: $time is formatted by strftime() and so be it. If then by usage of the command some inconsistency turns out, we can take a closer look into it. But I'd say it should just work this way, nothing complicated necessary (e.g. self::SECONDS_IN_MINUTE is bogus for correct time interval calculations when interested in iterating minutes correctly - imagine extra seconds for some days of the year- however on duplicates it's just continued two times on the same timestamp). Just do a good created at (that one is with seconds) and for the shedule w/o seconds. I've left a comment in review.

Generally it would be great if you could increase code-coverage. Code looks generally good, I've left some quick comments. If you want to already extract functionality, you can do so if you like but it's not a necessity. IMHO code coverage is more important so that it's possible to refactor more easily later on.

Thanks a lot for you PR. If you have any feedback on my feedback just shoot!

Contributor

ktomk commented Dec 5, 2016

Possible discussion points:

  1. The name schedule might be confusing, people might expect they can provide a timestamp on which time they want to schedule a job. Is this wanted? We could also name the command schedule-now or something like that?
    AFAIK we can't do that as in the end Magento decides when to execute it and it depends on how many there are. There is a timestamp so it can be set, but remeber it must not be executed at that time. It's something line "not before" that timestamp. Maybe defer this for a moment.

For the wording, please also take a look with Magerun 1 where - IIRC - a cron schedule command exists. In any case this should be backwards compatible. If this does not match, let's defer the details. But if you can, take a look.

  1. I copied a lot of code from the RunCommand class. This should definitely be refactored away. Should we put shared code in the AbstractCronCommand class, or someplace else?

If you can refactor it into a model class, I'd say it's preferable over an abstract base class as we should favor aggregation over inheritance. You can place such a model class next to those command classes (in the same directory). This might take more time than pushing members down to a new abstract-base-class. You're open to do it the way you prefer (or what currently works best for you), so no hard rules here. Historically, abstract classes were preferred, for me personally that's not my style but just find your way that is practically for you right now.

  1. There is something wrong with the timezone of the times in the RunCommand class. I haven't fixed this yet over there, but this new class should work correctly. I took a look at how M2 itself does this and based my code on that.
    I have no decision on this yet. I've seen the last ~3-5 days that Magento uses strftime which is locale dependent so I can imagine (but have not checked as only just seen) that locale might incorporate the timezone for the timestamp (sorry no definite check on it, references welcome). If that (locale and timezone dependent timestamp) is the case it would violate the best practice to use UTC based timestamps (not timezone dependent ones). So bringing inconsistencies in the mix (locale is not thread-safe) this might require the work-around you linked.

From this quick glance over the last days (was looking in there because of some Magerun 1 cron command), I'd say the use of strftime within Magento core definitely is a smell and I wondered why Magento core makes use of it for timestamps stored into the database system. Really.

For your question: I would prefer to not duplicate any work-arounds (if there are any) but instead do as documented: $time is formatted by strftime() and so be it. If then by usage of the command some inconsistency turns out, we can take a closer look into it. But I'd say it should just work this way, nothing complicated necessary (e.g. self::SECONDS_IN_MINUTE is bogus for correct time interval calculations when interested in iterating minutes correctly - imagine extra seconds for some days of the year- however on duplicates it's just continued two times on the same timestamp). Just do a good created at (that one is with seconds) and for the shedule w/o seconds. I've left a comment in review.

Generally it would be great if you could increase code-coverage. Code looks generally good, I've left some quick comments. If you want to already extract functionality, you can do so if you like but it's not a necessity. IMHO code coverage is more important so that it's possible to refactor more easily later on.

Thanks a lot for you PR. If you have any feedback on my feedback just shoot!

@@ -50,6 +57,7 @@ public function inject(
$this->_cronConfig = $cronConfig;
$this->_scopeConfig = $scopeConfig;
$this->_cronScheduleCollection = $cronScheduleCollection;
+ $this->_timezone = $timezone;

This comment has been minimized.

@ktomk

ktomk Dec 5, 2016

Contributor

Please convert ->... properties to ones w/o "" at the beginning. I know it's now all your code, so please at least do it for new introduced properties even if code-style changes that way (it changes to the correct way).

@ktomk

ktomk Dec 5, 2016

Contributor

Please convert ->... properties to ones w/o "" at the beginning. I know it's now all your code, so please at least do it for new introduced properties even if code-style changes that way (it changes to the correct way).

+ $schedule
+ ->setJobCode($jobCode)
+ ->setStatus(Schedule::STATUS_PENDING)
+ ->setCreatedAt(strftime('%Y-%m-%d %H:%M:%S', $this->_timezone->scopeTimeStamp()))

This comment has been minimized.

@ktomk

ktomk Dec 5, 2016

Contributor

created at with seconds, good.

@ktomk

ktomk Dec 5, 2016

Contributor

created at with seconds, good.

+ ->setJobCode($jobCode)
+ ->setStatus(Schedule::STATUS_PENDING)
+ ->setCreatedAt(strftime('%Y-%m-%d %H:%M:%S', $this->_timezone->scopeTimeStamp()))
+ ->setScheduledAt($scheduledAtTime)

This comment has been minimized.

@ktomk

ktomk Dec 5, 2016

Contributor

I think this needs to be a formatted timestamp as well, correct me if not.

@ktomk

ktomk Dec 5, 2016

Contributor

I think this needs to be a formatted timestamp as well, correct me if not.

+ */
+ protected function askJobCode(InputInterface $input, OutputInterface $output, $jobs)
+ {
+ foreach ($jobs as $key => $job) {

This comment has been minimized.

@ktomk

ktomk Dec 5, 2016

Contributor

Sadly we don't offer anything less cumbersome here. This might be something worth in a base-class if shared with the other command as asking is command related (less cron sheduling related, at least for most of this code).

@ktomk

ktomk Dec 5, 2016

Contributor

Sadly we don't offer anything less cumbersome here. This might be something worth in a base-class if shared with the other command as asking is command related (less cron sheduling related, at least for most of this code).

@hostep

This comment has been minimized.

Show comment
Hide comment
@hostep

hostep Dec 26, 2016

Contributor

Hi @tkn98

Sorry for the delay, but I just found time to update this PR and I think I addressed most of your remarks. If you have new remarks, please let me know.

If you can refactor it into a model class, I'd say it's preferable over an abstract base class as we should favor aggregation over inheritance.

I chose to move the askJobCode method to the AbstractCronCommand class, because it would take me to long to refactor it decently using composition over inheritance.

For the wording, please also take a look with Magerun 1 where - IIRC - a cron schedule command exists. In any case this should be backwards compatible. If this does not match, let's defer the details. But if you can, take a look.

As far as I can see, there is no such command in Magerun 1, so the naming is still open for discussion :)

Contributor

hostep commented Dec 26, 2016

Hi @tkn98

Sorry for the delay, but I just found time to update this PR and I think I addressed most of your remarks. If you have new remarks, please let me know.

If you can refactor it into a model class, I'd say it's preferable over an abstract base class as we should favor aggregation over inheritance.

I chose to move the askJobCode method to the AbstractCronCommand class, because it would take me to long to refactor it decently using composition over inheritance.

For the wording, please also take a look with Magerun 1 where - IIRC - a cron schedule command exists. In any case this should be backwards compatible. If this does not match, let's defer the details. But if you can, take a look.

As far as I can see, there is no such command in Magerun 1, so the naming is still open for discussion :)

@ktomk ktomk merged commit 5f081f7 into netz98:develop Dec 31, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ktomk

This comment has been minimized.

Show comment
Hide comment
@ktomk

ktomk Dec 31, 2016

Contributor

Thanks, and sorry for the delay, too :) This is now in the develop version.

And for the name: I don't know where I took that from, so, no discussion at all ^ ^

Contributor

ktomk commented Dec 31, 2016

Thanks, and sorry for the delay, too :) This is now in the develop version.

And for the name: I don't know where I took that from, so, no discussion at all ^ ^

cmuench added a commit to cmuench/n98-magerun2 that referenced this pull request Oct 11, 2017

Add sys:cron:schedule command, closes #257
Add a command called `sys:cron:schedule` to schedule cronjobs so they get picked up by the next cron run.

Motivation: Sometimes we want to run a cronjob, but not keep our ssh session or terminal open while running sys:cron:run. 

Was inspired by the excellent Aoe_Scheduler module for M1.

Additional to the new feature some detailed changes regarding code-style and stability / detail fixes that overall improve the command incl. the base class:

* Move the askJobCode method to the cron command base class.

* Format the scheduled at time, without seconds, just like Magento does this.

* Add a basic test for the new sys:cron:schedule command.

* Remove underscores in front of the members in all cron related classes. Also remove the unused 'eventManager' member.

* Remove some more unused members and a const from the cron code (and even more unused members).

* Fix the timezone of the timestamps of the sys:cron:run command to match Magento's behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment