Skip to content

the enqueue_time in context can use the direct timezone #281

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

Merged
merged 19 commits into from
Mar 15, 2022
Merged

the enqueue_time in context can use the direct timezone #281

merged 19 commits into from
Mar 15, 2022

Conversation

ponytailer
Copy link
Contributor

@ponytailer ponytailer commented Dec 1, 2021

the enqueue_time in context is still utc, it's not good.

@ponytailer
Copy link
Contributor Author

@samuelcolvin please

@samuelcolvin
Copy link
Member

I think I would rather stick to all timestamps being UTC, I don't want to add more complexity and depdencies unless absolutely necessary.

Please can you provide an explanation of why you need this and how you will use it beyond "it's not good.".

@ponytailer
Copy link
Contributor Author

ponytailer commented Jan 26, 2022

I think I would rather stick to all timestamps being UTC, I don't want to add more complexity and depdencies unless absolutely necessary.

Please can you provide an explanation of why you need this and how you will use it beyond "it's not good.".

In my country,the default timezone is not the utc。So when the job is processing,the arq log(called ms_to_datetime)was less 8 hours。 Not easy to read for people who is not the programmer。 Thanks for your reply。

@samuelcolvin
Copy link
Member

In my country, the default timezone is not UTC either. :-) Arguably it's even worse, because the timezone IS utc for half the year, then UTC+1 for the other half.

Still, as far as I know arq only shows time differences in logs. If you want to log datetimes, in your own timezone you can implement that in your own code without modifying arq.

I'm inclined to close this, unless there's some piece of code I'm missing that lots datetimes and would require a change to the code base to log correctly.

@ponytailer
Copy link
Contributor Author

ponytailer commented Jan 26, 2022

In my country, the default timezone is not UTC either. :-) Arguably it's even worse, because the timezone IS utc for half the year, then UTC+1 for the other half.

Still, as far as I know arq only shows time differences in logs. If you want to log datetimes, in your own timezone you can implement that in your own code without modifying arq.

I'm inclined to close this, unless there's some piece of code I'm missing that lots datetimes and would require a change to the code base to log correctly.

but when the job was finished or other status。arq will print the log include the utc datetime,it's not good for reading。
unless write my own arq worker class extends Worker and rewrite it complexly。Or ignore that arq log。

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm prepared to accept this change, but this pull request needs a lot of work.

ponytailer and others added 3 commits March 7, 2022 15:48

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #281 (0081a03) into master (22ee862) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
- Coverage   98.84%   98.76%   -0.08%     
==========================================
  Files          11       11              
  Lines         951      972      +21     
  Branches      161      165       +4     
==========================================
+ Hits          940      960      +20     
  Misses          6        6              
- Partials        5        6       +1     
Impacted Files Coverage Δ
arq/constants.py 100.00% <100.00%> (ø)
arq/utils.py 100.00% <100.00%> (ø)
arq/worker.py 99.33% <0.00%> (-0.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22ee862...0081a03. Read the comment docs.

@ponytailer
Copy link
Contributor Author

Okay, I'm prepared to accept this change, but this pull request needs a lot of work.

Thanks for your review.

Verified

This commit was signed with the committer’s verified signature.
samuelcolvin Samuel Colvin

Verified

This commit was signed with the committer’s verified signature.
samuelcolvin Samuel Colvin

Verified

This commit was signed with the committer’s verified signature.
samuelcolvin Samuel Colvin

Verified

This commit was signed with the committer’s verified signature.
samuelcolvin Samuel Colvin

Verified

This commit was signed with the committer’s verified signature.
samuelcolvin Samuel Colvin
@samuelcolvin samuelcolvin merged commit c3ac93e into python-arq:master Mar 15, 2022
@samuelcolvin
Copy link
Member

great, thanks so much.

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 this pull request may close these issues.

None yet

2 participants