-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Slackbot optimization #3696
Slackbot optimization #3696
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
General comment in glancing through the code ... would think about reducing the class member side effects of some of these methods ... if the functions can be made pure by adding just one or a few parameters and/or returning state, we should do that where possible to make it easier to reason through.
@@ -252,42 +268,111 @@ def acquire_tenants(self) -> None: | |||
ex=TENANT_LOCK_EXPIRATION, |
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.
Maybe we should add comments to this to help explain how the lock is being used.
For the above lock, why manually set the lock vs using the lock class? it's a lot more error prone to have to handle the lock manually. Are we persisting the lock across runs? When does it get deleted?
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.
Great point- inherited but 100% agree re lock stability / use
68589fd
to
48f82eb
Compare
48f82eb
to
7562113
Compare
Description
Fixes https://linear.app/danswer/issue/DAN-1275/slack-bot-not-working-with-on-our-cloud
Fixes cloud slackbot "acquiring" many tenants
How Has This Been Tested?
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.