-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(tokens): Add async flush outboxes #105264
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
Conversation
src/sentry/models/apitoken.py
Outdated
|
|
||
| # Schedule async replication if using async mode | ||
| if not self._should_flush_outbox(): | ||
| transaction.on_commit( |
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.
ok I'm not actually sure if this does anything since we've already completed all the prev save steps? But I saw this being used for drain_shard and we'd want to enqueue the replication task after the token has committed 🤷♀️
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.
Because you're scheduling a task, it is best to do that after the transaction has commit as you can ensure that all the records are saved. Without this it is possible for the task to be processed while the transaction has not complete if postgres is slow and kafka is fast.
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
src/sentry/models/apitoken.py
Outdated
| has_async_flush = self.user_id in options.get("users:api-token-async-flush") | ||
| logger.info( | ||
| "async_flush_check", | ||
| extra={ | ||
| "has_async_flush": has_async_flush, | ||
| "user_id": self.user_id, | ||
| "token_id": self.id, | ||
| }, | ||
| ) | ||
| if has_async_flush: | ||
| return False | ||
|
|
||
| return True |
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.
Discussed in person, but I don't think we need a user-level flag for this. I think a global option that sets the value of the class's default_flush property should be sufficient for us. Having an all-or-nothing toggle is probably okay for this change, and I don't expect the majority of users to even notice the impact of this change.
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.
Setting class properties based on options will be tricky as classes are imported during django's startup before the ORM is ready. The current approach of overriding _maybe_prepare_outboxes will work though.
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.
ended up doing some property decorator shenanigans since that should be loaded after the startup 🤔
src/sentry/models/apitoken.py
Outdated
| drain_outbox_shards_control.delay( | ||
| outbox_identifier_low=first_row.id, | ||
| outbox_identifier_hi=last_row.id + 1, | ||
| outbox_name="sentry.ControlOutbox", | ||
| ) |
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.
I wonder if we should move this entire function to the parent ControlOutbox or even Outbox class to trigger instead. Being able to enqueue a task to more quickly drain a shard seems like a useful feature in other contexts, beyond just this one.
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.
We could add a new task that accepts the shard_scope, shard_identifier, category, and object_identifier as parameters, and that task could do the range query + call process_outbox_batch() 🤷
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.
slapped it into the parent ControlOutboxProducingModel
src/sentry/models/apitoken.py
Outdated
|
|
||
| # Schedule async replication if using async mode | ||
| if not self._should_flush_outbox(): | ||
| transaction.on_commit( |
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.
Because you're scheduling a task, it is best to do that after the transaction has commit as you can ensure that all the records are saved. Without this it is possible for the task to be processed while the transaction has not complete if postgres is slow and kafka is fast.
src/sentry/models/apitoken.py
Outdated
| has_async_flush = self.user_id in options.get("users:api-token-async-flush") | ||
| logger.info( | ||
| "async_flush_check", | ||
| extra={ | ||
| "has_async_flush": has_async_flush, | ||
| "user_id": self.user_id, | ||
| "token_id": self.id, | ||
| }, | ||
| ) | ||
| if has_async_flush: | ||
| return False | ||
|
|
||
| return True |
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.
Setting class properties based on options will be tricky as classes are imported during django's startup before the ORM is ready. The current approach of overriding _maybe_prepare_outboxes will work though.
src/sentry/models/apitoken.py
Outdated
| drain_outbox_shards_control.delay( | ||
| outbox_identifier_low=first_row.id, | ||
| outbox_identifier_hi=last_row.id + 1, | ||
| outbox_name="sentry.ControlOutbox", | ||
| ) |
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.
We could add a new task that accepts the shard_scope, shard_identifier, category, and object_identifier as parameters, and that task could do the range query + call process_outbox_batch() 🤷
markstory
left a comment
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.
Looks correct to me.
oh man this one's a doozy