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

etcd jobstore support for apscheduler 3.x #777

Merged
merged 11 commits into from
Aug 20, 2023
Merged

Conversation

Yan-Daojiang
Copy link

Recently, many people use etcd as their metadata hub, so I tried adding etcd persistent job storage for Apscheduler 3.x. I added the corresponding unit tests with 100% coverage, and they passed the tests locally.

@Yan-Daojiang Yan-Daojiang reopened this Aug 17, 2023
Copy link
Owner

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

I wouldn't have entertained the idea of adding a new back-end to the 3.x series but this PR was exceptionally well prepared, so I'm giving it a pass. I left some minor comments which should be addressed before merging though.

apscheduler/jobstores/etcd.py Outdated Show resolved Hide resolved
apscheduler/jobstores/etcd.py Outdated Show resolved Hide resolved
apscheduler/jobstores/etcd.py Outdated Show resolved Hide resolved
docs/contributing.rst Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
apscheduler/jobstores/etcd.py Outdated Show resolved Hide resolved
@Yan-Daojiang
Copy link
Author

Hi agronholm! Thank you very much for your review of my code and your valuable suggestions, I made some changes to them.
I noticed that your focus may be on 4.x at the moment, and some important changes have been made. I'd love to be able to make some contributions on 4.x in the future. Since pre-release 4.x is not currently in production, I only contributed to 3.x.

@Yan-Daojiang
Copy link
Author

Yan-Daojiang commented Aug 20, 2023

There seems to be a problem in executor. This should not be caused by the new back-end. Could you help to take a look?
When runing the test case test_broken_pool , the default memory storage should be used.

@agronholm
Copy link
Owner

There seems to be a problem in executor. This should not be caused by the new back-end. Could you help to take a look? When runing the test case test_broken_pool , the default memory storage should be used.

test_broken_pool is known to be flaky, and there's an open issue about it.

@Yan-Daojiang
Copy link
Author

OK, thank you! I have no other questions.

@agronholm agronholm merged commit 677ce71 into agronholm:3.x Aug 20, 2023
5 checks passed
@Yan-Daojiang Yan-Daojiang deleted the dev branch August 21, 2023 02:26
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.

2 participants