Make last_enqueue_time be always an instance of Time.

[ryotarai]

On my laptop in Japanese-language environment, loading @last_enqueue_time raises a parse error:

2022-08-08T05:59:06.992Z pid=58672 tid=2kqk WARN: Date::Error: invalid date
2022-08-08T05:59:06.992Z pid=58672 tid=2kqk WARN: /Users/ryotarai/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/sidekiq-cron-1.7.0/lib/sidekiq/cron/job.rb:632:in `parse'
/Users/ryotarai/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/sidekiq-cron-1.7.0/lib/sidekiq/cron/job.rb:632:in `rescue in parse_enqueue_time'
/Users/ryotarai/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/sidekiq-cron-1.7.0/lib/sidekiq/cron/job.rb:629:in `parse_enqueue_time'
/Users/ryotarai/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/sidekiq-cron-1.7.0/lib/sidekiq/cron/job.rb:285:in `initialize'

This PR makes @last_enqueue_time be always Time instance.

[markets]

Hi @ryotarai and thanks for contributing here!

To be honest I'm not sure about this change, I have a couple of questions. Is there a place in the code where we can set the @last_enqueue_time.strftime(LAST_ENQUEUE_TIME_FORMAT) only once? And second one, are these changes compatible with already existing jobs?

[danilogco]

Wouldn't it be enough if you just changed the to_hash method?

[serprex]

@markets it seems backwards compatible since this is explicitly formatting timestamps with the format we try first before falling back to DateTime.parse, which remains for deserializing existing jobs

Could make a method formatted_last_enqueue_time

[markets]

Could make a method

That would be 👌

[markets]

hi @ryotarai 👋🏼 Do you mind to update from master and address @danilogco @serprex comments?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[serprex]

not stale

[farooqch11]

Waiting for this PR , as i am using my own fork, but it seems like generating a new issue on production after adding same code mentioned in this branch @markets

vendor/bundle/ruby/3.0.0/gems/redis-client-0.12.1/lib/redis_client/command_builder.rb:37:in `block in generate': Unsupported command argument type: Time (TypeError) from vendor/bundle/ruby/3.0.0/gems/redis-client-0.12.1/lib/redis_client/command_builder.rb:28:in `map!' from vendor/bundle/ruby/3.0.0/gems/redis-client-0.12.1/lib/redis_client/command_builder.rb:28:in `generate' from vendor/bundle/ruby/3.0.0/gems/redis-client-0.12.1/lib/redis_client.rb:207:in `call' from vendor/bundle/ruby/3.0.0/gems/sidekiq-7.0.3/lib/sidekiq/redis_client_adapter.rb:27:in `method_missing' from vendor/bundle/ruby/3.0.0/bundler/gems/sidekiq-cron-6150a944f37e/lib/sidekiq/cron/job.rb:475:in `block in save_last_enqueue_time' from vendor/bundle/ruby/3.0.0/gems/sidekiq-7.0.3/lib/sidekiq/config.rb:156:in `block in redis' from vendor/bundle/ruby/3.0.0/gems/connection_pool-2.3.0/lib/connection_pool.rb:65:in `block (2 levels) in with' from vendor/bundle/ruby/3.0.0/gems/connection_pool-2.3.0/lib/connection_pool.rb:64:in `handle_interrupt' from vendor/bundle/ruby/3.0.0/gems/connection_pool-2.3.0/lib/connection_pool.rb:64:in `block in with' from vendor/bundle/ruby/3.0.0/gems/connection_pool-2.3.0/lib/connection_pool.rb:61:in `handle_interrupt' from vendor/bundle/ruby/3.0.0/gems/connection_pool-2.3.0/lib/connection_pool.rb:61:in `with' from vendor/bundle/ruby/3.0.0/gems/sidekiq-7.0.3/lib/sidekiq/config.rb:153:in `redis' from vendor/bundle/ruby/3.0.0/gems/sidekiq-7.0.3/lib/sidekiq.rb:73:in `redis' from vendor/bundle/ruby/3.0.0/bundler/gems/sidekiq-cron-6150a944f37e/lib/sidekiq/cron/job.rb:473:in `save_last_enqueue_time' from vendor/bundle/ruby/3.0.0/bundler/gems/sidekiq-cron-6150a944f37e/lib/sidekiq/cron/job.rb:72:in `enque!' from vendor/bundle/ruby/3.0.0/bundler/gems/sidekiq-cron-6150a944f37e/lib/sidekiq/cron/job.rb:40:in `test_and_enque_for_time!' from vendor/bundle/ruby/3.0.0/bundler/gems/sidekiq-cron-6150a944f37e/lib/sidekiq/cron/poller.rb:36:in `enqueue_job' from vendor/bundle/ruby/3.0.0/bundler/gems/sidekiq-cron-6150a944f37e/lib/sidekiq/cron/poller.rb:23:in `block in enqueue' from vendor/bundle/ruby/3.0.0/bundler/gems/sidekiq-cron-6150a944f37e/lib/sidekiq/cron/poller.rb:22:in `each' from vendor/bundle/ruby/3.0.0/bundler/gems/sidekiq-cron-6150a944f37e/lib/sidekiq/cron/poller.rb:22:in `enqueue' from vendor/bundle/ruby/3.0.0/gems/sidekiq-7.0.3/lib/sidekiq/scheduled.rb:99:in `block in start' from vendor/bundle/ruby/3.0.0/gems/sidekiq-7.0.3/lib/sidekiq/component.rb:8:in `watchdog' from vendor/bundle/ruby/3.0.0/gems/sidekiq-7.0.3/lib/sidekiq/component.rb:17:in `block in safe_thread'

[serprex]

@farooqch11 Your stacktrace is a bit odd. For one, save_last_enqueue_time should be at line 480, but your stacktrace is saying 473. Secondly, PR has conn.hset redis_key, 'last_enqueue_time', @last_enqueue_time.strftime(LAST_ENQUEUE_TIME_FORMAT) which should be avoiding sending raw time value error is complaining about

[farooqch11]

@serprex is my code , can you please out to me, basically i am not sending Raw time. instead rescue to nil https://github.com/sidekiq-cron/sidekiq-cron/compare/master...farooqch11:sidekiq-cron:master

[serprex]

@farooqch11 You didn't update save_last_enqueue_time

[markets]

Hello @ryotarai did you have the chance to take a look to those comments? I'd like to include this fix in the next release.

If you don't have time right now, maybe @farooqch11 or @serprex can take it over?

[ryotarai]

Hi. Sorry for late response.

I've resolved conflict and commited https://github.com/sidekiq-cron/sidekiq-cron/pull/354/commits/c00fb144a19e9d6f812b8bc474277e73f8bca724 to deal with this change.

are these changes compatible with already existing jobs?

Yes, this does not break compatibility.

[farooqch11]

Cool, now its ready for master ?

On Thu, 2 Mar 2023 at 8:08 PM, Marc Anguera @.***> wrote:

Merged #354 https://github.com/sidekiq-cron/sidekiq-cron/pull/354 into master.

— Reply to this email directly, view it on GitHub https://github.com/sidekiq-cron/sidekiq-cron/pull/354#event-8650446793, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIFWI2YMGSCQOZMIGW5GPM3W2CZW5ANCNFSM5535JJDQ . You are receiving this because you were mentioned.Message ID: @.***>

-- [image: logo-1.png] Farooq Ashraf Founder @.*** @.***> | +923347188223 stackworx.co

[markets]

Yes @farooqch11! Please, in case you can test it out (from the canonical master branch) any feedback is really appreciated.

[farooqch11]

Sure, i will start using this on production.

On Thu, 2 Mar 2023 at 8:12 PM, Marc Anguera @.***> wrote:

Yes @farooqch11 https://github.com/farooqch11! Please, in case you can test it out (from the canonical master branch) any feedback is really appreciated.

— Reply to this email directly, view it on GitHub https://github.com/sidekiq-cron/sidekiq-cron/pull/354#issuecomment-1452033242, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIFWI23PC5TR5MBSBPP6KPDW2C2HNANCNFSM5535JJDQ . You are receiving this because you were mentioned.Message ID: @.***>

-- [image: logo-1.png] Farooq Ashraf Founder @.*** @.***> | +923347188223 stackworx.co