Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cron jobs and timezone handling #136

Closed
IgnisDa opened this issue Jul 31, 2023 · 33 comments · Fixed by #178
Closed

Cron jobs and timezone handling #136

IgnisDa opened this issue Jul 31, 2023 · 33 comments · Fixed by #178

Comments

@IgnisDa
Copy link

IgnisDa commented Jul 31, 2023

I have a docker container which inherits from scratch. I have an apalis cron job which is supposed to execute at 0000 hrs and 1200 hrs everyday. It does execute but at exactly 0530 hrs for me (I live in Asia/Kolkata timezone which has that offset). How can I configure apalis so that it executes at 0000 hrs of my timezone?

Setting the TZ env variable does not work, probably due to the scratch docker environment.

I'm not really sure if this problem even belongs to this repository, but would love any pointers you'd have on debugging this issue.

@geofmureithi
Copy link
Owner

Currently apalis uses UTC.
https://docs.rs/apalis-cron/latest/src/apalis_cron/lib.rs.html#108
Is it possible you play around with a custom CronStream implementation? And then you can comment here what you find out then we shall see what solution is possible.

@IgnisDa
Copy link
Author

IgnisDa commented Jul 31, 2023

Any suggestions on how I can test it? My first attempt will be replace Utc::now() and load it from TZ env var using chrono_tz. But how can I actually see in what timezone offset the function is being executed?

@geofmureithi
Copy link
Owner

geofmureithi commented Aug 1, 2023

I think UTC is standard, and based on the machine's time settings. Are you sure that is ok?
Anyways, I will try something and get back.

@IgnisDa
Copy link
Author

IgnisDa commented Aug 1, 2023

Yes that behaviour is expected. However I'm running my application in an environment which does not have the timezone database (docker scratch).

@geofmureithi
Copy link
Owner

Could you give this a try?

RUN apt-get update && apt-get install -y tzdata
ENV TZ=Your/Timezone
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone

Not sure it will work but just throwing it out there

@geofmureithi
Copy link
Owner

geofmureithi commented Aug 1, 2023

I think there is a fix that can help though.
Since cron offers a chance to set the timezone,

   pub fn after_owned<Z: TimeZone>(&self, after: DateTime<Z>) -> OwnedScheduleIterator<Z> {
        OwnedScheduleIterator::new(self.clone(), after)
    }

I will offer the same functionality as here:

 pub fn to_stream_with_timezone<T: Timezone>(self) -> BoxStream { .. }

@IgnisDa
Copy link
Author

IgnisDa commented Aug 1, 2023

Could you give this a try?

RUN apt-get update && apt-get install -y tzdata
ENV TZ=Your/Timezone
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone

Not sure it will work but just throwing it out there

Can't do this since this has to be set at build time, and I want users to be able to configure it at start-time.

I think there is a fix that can help though. Since cron offers a chance to set the timezone,

   pub fn after_owned<Z: TimeZone>(&self, after: DateTime<Z>) -> OwnedScheduleIterator<Z> {
        OwnedScheduleIterator::new(self.clone(), after)
    }

I will offer the same functionality as here:

 pub fn to_stream_with_timezone<T: Timezone>(self) -> BoxStream { .. }

Yeah, looks good to me!

@geofmureithi
Copy link
Owner

Awesome, this should be fixed in the next release.
On another note, I want to make a list of projects using apalis, hope I can include ryot there :)

@IgnisDa
Copy link
Author

IgnisDa commented Aug 1, 2023

Awesome, this should be fixed in the next release.

What changes would I have to make?

hope I can include ryot there :)

I would be honored!

@geofmureithi
Copy link
Owner

What changes would I have to make?

I will document a new api, it will be in the release changelog.
You will update to the new api and will possibly need to add an env variable for the timezone config

@IgnisDa
Copy link
Author

IgnisDa commented Aug 1, 2023

Cool makes sense. Thanks for your prompt response.

@IgnisDa
Copy link
Author

IgnisDa commented Aug 22, 2023

Hey @geofmureithi any updates on this?

@geofmureithi
Copy link
Owner

Hopefully by the end of this month. OSS doesn't pay, I still have to pay my bills you know.

@IgnisDa
Copy link
Author

IgnisDa commented Aug 22, 2023

Sorry if I came off as entitled. I understand.

@geofmureithi
Copy link
Owner

Ha, not really entitled. I should have added a smile emoji at the end. (Its just that my focus has been on other projects)
You are also building OSS stuff, I know you know.
Anyways, I did not prioritise this issue because I didn't consider it blocking. But it is a straight forward one. So yeah, this will be resolved over the weekend.

@IgnisDa
Copy link
Author

IgnisDa commented Sep 24, 2023

Hi @geofmureithi. Were you able to get to this?

@geofmureithi
Copy link
Owner

Here is a quick solution for your case.

trait ToStreamWithTimezone {
    fn to_stream_with_timezone(self, tz: Timezone) -> BoxStream;
}

Then implement it for CronStream.
This is a simple workaround and can unblock you until this issue is fixed.
Let me know if this helps.

@IgnisDa
Copy link
Author

IgnisDa commented Sep 25, 2023

I am not able to figure out how to use it. Is there any example?

@geofmureithi
Copy link
Owner

This is how the implementation would look:

impl<J: From<DateTime<TZ>> + Job + Send + 'static, T: Timer + Sync + Send + 'static, TZ> ToStreamWithTimezone for
    CronStream<J, T>
{
   fn to_stream_with_timezone(self, tz: TZ) -> BoxStream<'static, Result<Option<JobRequest<J>>, JobError>> {
        let stream = async_stream::stream! {
            let mut schedule = self.0.upcoming_owned(tz);
            loop {
                let next = schedule.next();
                match next {
                    Some(next) => {
                        let to_sleep = next - DateTime::<TZ>::now();
                        let to_sleep = to_sleep.to_std().map_err(|e| JobError::Failed(e.into()))?;
                        self.2.sleep(to_sleep).await;
                        yield Ok(Some(JobRequest::new(J::from(DateTime::<TZ>::now()))));
                    },
                    None => {
                        yield Ok(None);
                    }
                }

            }
        };
        stream.boxed()
    }
}

Let me know if it works, I haven't tested it.

@IgnisDa
Copy link
Author

IgnisDa commented Sep 26, 2023

I could not get it working: IgnisDa/ryot@ae2987f

@geofmureithi
Copy link
Owner

I could not get it working

What was the error? Let me try to run it locally.

@IgnisDa
Copy link
Author

IgnisDa commented Sep 26, 2023

I could not get generics working
image

@geofmureithi
Copy link
Owner

@IgnisDa Please check if this solution works for you. #178

@IgnisDa
Copy link
Author

IgnisDa commented Oct 6, 2023

@geofmureithi I was not able to figure out how to use it. I tried it with chrono_tz::Asia::Kolkata but it did not work.

image

Here are the changes I made IgnisDa/ryot@bbb8bdd.

@geofmureithi
Copy link
Owner

The compiler is telling you of a missing From<DateTime<Tz>> implementation here

impl From<DateTimeUtc> for ScheduledJob {
    fn from(value: DateTimeUtc) -> Self {
        Self(value)
    }
}

@IgnisDa
Copy link
Author

IgnisDa commented Oct 6, 2023

@geofmureithi Does not work.
image
IgnisDa/ryot@4c09f55

@geofmureithi
Copy link
Owner

Give it a try now.

@IgnisDa
Copy link
Author

IgnisDa commented Oct 6, 2023

@geofmureithi Yep this works as expected. I think it can be merged.

@IgnisDa
Copy link
Author

IgnisDa commented Oct 8, 2023

@geofmureithi Could you also publish to crates.io?

@geofmureithi
Copy link
Owner

Yeah on it

@IgnisDa
Copy link
Author

IgnisDa commented Oct 8, 2023

Thank you very much for fixing this issue.

@IgnisDa
Copy link
Author

IgnisDa commented Oct 9, 2023

@geofmureithi Were you able to get around to publishing it?

@geofmureithi
Copy link
Owner

#181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants