ref(reprocessing): Retain issue data by migrating associated models over #21609
Conversation
| _REDIS_SYNC_TTL = 3600 | ||
|
|
||
|
|
||
| GROUP_MODELS_TO_MIGRATE = GROUP_RELATED_MODELS + (models.Activity,) |
There was a problem hiding this comment.
After #21611 this addition should not be necessary
size-limit report
|
|
This PR has a migration; here is the generated SQL --
-- Alter field type on activity
-- |
| group.short_id = None # satisfy unique constraint | ||
| group.save() | ||
|
|
||
| group.pk = group.id = None |
There was a problem hiding this comment.
Might be worth having a comment here that you're tricking django into doing an INSERT.
| project_id=group.project_id, | ||
| group_id=group.id, | ||
| max_events=max_events, | ||
| acting_user_id=request.user.id, |
There was a problem hiding this comment.
Let's be very careful with this. The user can be a Sentry app or also None, in case this is being executed via certain auth tokens. It's probably worth checking for the existence of a user before accessing .id.
There was a problem hiding this comment.
Are we sure there's no request user in certain situations? I thought auth tokens are attached to users. It's kinda undesirable to have no idea who triggered a reprocessing.
There was a problem hiding this comment.
jan says request.user could be SentryApp, fixed
There was a problem hiding this comment.
fwiw I quickly tried setting up integration platform locally to test this, but I failed (getting csrf issues)
| from ..base import BaseDeletionTask, BaseRelation, ModelDeletionTask, ModelRelation | ||
|
|
||
|
|
||
| GROUP_RELATED_MODELS = ( |
There was a problem hiding this comment.
nit: Since this is now used outside of deletions, can we move this to a better place, for instance somewhere in sentry.models? This could even be on Group itself.
There was a problem hiding this comment.
Despite the name this is still related to group deletion and the order of items is specific to deletion. I was also considering duplicating the list like group merge/unmerge does. Let me know what you prefer.
There was a problem hiding this comment.
You're right, I'll leave this up to you. I found it interesting that this is being imported from reprocessing to copy over information rather than delete it, that's all.
src/sentry/reprocessing2.py
Outdated
| transferred_group_attrs[attr] = getattr(group, attr) | ||
|
|
||
| group.status = models.GroupStatus.REPROCESSING | ||
| group.short_id = None # satisfy unique constraint |
There was a problem hiding this comment.
Note that this relies on Postgres specific behavior, which is fine now that we only support postgres.
There was a problem hiding this comment.
what's the pg specific behavior? I can add a comment
There was a problem hiding this comment.
That NULL is excluded from unique constraints. AFAIK, MSSQL does enforce this, but I might be wrong.
There was a problem hiding this comment.
Ah yes. I added a comment. This slipped past me.
| def start_group_reprocessing(project_id, group_id, max_events=None, acting_user_id=None): | ||
| from django.db import transaction | ||
|
|
||
| with transaction.atomic(): |
There was a problem hiding this comment.
btw this might be a pretty large transaction, it touches a lot of tables at once.
There was a problem hiding this comment.
Let's follow up on this please. I'm pretty sure we'll want to back this out.
There was a problem hiding this comment.
The problem is that I would really not like to fail halfway through, because that's quite a mess to clean up. Perhaps we want some shitty kind of "clientside rollback", I think I saw that someplace else already.
| def start_group_reprocessing(project_id, group_id, max_events=None, acting_user_id=None): | ||
| from django.db import transaction | ||
|
|
||
| with transaction.atomic(): |
There was a problem hiding this comment.
Let's follow up on this please. I'm pretty sure we'll want to back this out.
Works on my machine!