-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add leaderelection module #347
base: master
Are you sure you want to change the base?
Conversation
I started off by adapting the leaderelection module from |
33c2ad8
to
c7de87d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #347 +/- ##
==========================================
+ Coverage 26.91% 27.05% +0.13%
==========================================
Files 799 805 +6
Lines 98316 98706 +390
==========================================
+ Hits 26463 26704 +241
- Misses 71853 72002 +149 ☔ View full report in Codecov by Sentry. |
No worries, I usually adapt with modifications. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent job, as always.
Thank you, Jacob.
c7de87d
to
112c3d4
Compare
Add leaderelection module, based off of the leaderelection module in kubernetes-client/python. The module has been altered slightly to support asyncio. Fixes tomplus#297
112c3d4
to
cce9f8b
Compare
Add leaselock, derived from [1]. [1]: https://github.com/kubernetes-client/python/pull/2314/files
|
I agree, some of them should be on debug level... |
Instead of passing Callable[...,Coroutine] functions as callbacks, pass Coroutines directly. This allows the caller to supply arbitrary context associated with the Coroutine, such as the ApiClient used to establish the leader election.
* Remove misplaced basicConfig * Use %-style string interpolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
I switched to a mix of debug for verbose debug conditions, and error/exception for messages that actually indicate problems. Let me know if you agree with the selections. I also made an unrelated change in ef01ee6, switching the callback functions from async with api_client.ApiClient() as apic:
# Create config
leader_election_config = electionconfig.Config(
# A legacy ConfigMapLock is also available
LeaseLock(lock_name, lock_namespace, candidate_id, apic),
lease_duration=17,
renew_deadline=15,
retry_period=5,
onstarted_leading=example_start_func(api_client=apic),
onstopped_leading=example_end_func(),
)
# Enter leader election
await leaderelection.LeaderElection(leader_election_config).run() This seemed to be much more pleasant than other options for including arbitrary context, such as defining otherwise-unnecessary wrapper functions, using contextvars, etc. Let me know what you think. |
I see two potential problems with passing coroutines:
what about supporting two versions? we can check if it is a function or a coroutine and act accordingly. |
For the function version, are you suggesting supporting non-async functions, or supporting async functions (that return coroutines)? |
async functions and coroutines:
|
Add leaderelection module, based off of the leaderelection module in kubernetes-client/python. The module has been altered slightly to support asyncio.
Fixes #297