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

Search/ImportedFile: rename build to sync_id #10734

Open
stsewd opened this issue Sep 13, 2023 · 1 comment
Open

Search/ImportedFile: rename build to sync_id #10734

stsewd opened this issue Sep 13, 2023 · 1 comment
Labels
Improvement Minor improvement to code

Comments

@stsewd
Copy link
Member

stsewd commented Sep 13, 2023

What's the problem this feature will solve?

Currently our ImportedFile model and Page index have an attribute named build, which is the build id from where the files are generated.

build = models.IntegerField(_('Build id'), null=True)

We use this ID not to track the build itself, but to allow us to delete the previous objects after we have successfully created the previous ones. The build ID works great, except that in case of a re-index we won't trigger a new build, so the build ID won't be changed, leaving no way for us to identity the old files from the new ones.

Describe the solution you'd like

Instead of tracking the build ID, just have a new field specific for the purpose of tracking the files generated from the current task instead of the build.

And instead of using a numeric field, we can probably just use a random string, which is easier to generate without having to query for the current sync number, and also has fewer chances of colliding with another task. Can also just be the current date.

To avoid downtimes, we won't "rename" the field, but just create a new one, and use that instead of build, and after the next deploy we can remove it.

Alternative solutions

Just keep the field named build, even if it doesn't exactly maps to a build.

Additional context

#10696

@stsewd stsewd added the Improvement Minor improvement to code label Sep 13, 2023
@humitos
Copy link
Member

humitos commented Sep 14, 2023

And instead of using a numeric field, we can probably just use a random string, which is easier to generate without having to query for the current sync number, and also has fewer chances of colliding with another task. Can also just be the current date.

I think we can use a UUIDField for this.

Just keep the field named build, even if it doesn't exactly maps to a build.

Naming things that mean something different than its name it's pretty confusing. I'd prefer if we change them. While reviewing the PR I thought it was a pretty dark hack or even an error. Without your explanation I would have been thinking the same. I'm sure that next time I read this code I will think it's broken if we don't rename the field 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
None yet
Development

No branches or pull requests

2 participants