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

Forced photometry update pipeline - missing match stage #264

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

Theodlz
Copy link
Collaborator

@Theodlz Theodlz commented Dec 13, 2023

That had been hot patched early on in production (without it, it just got stuck and didn't even finish running any query anyway, so none of the resulting data in the DB has been affected), but adding it here to not forget later on.

@Theodlz Theodlz requested a review from mcoughlin December 13, 2023 16:46
Copy link
Collaborator

@mcoughlin mcoughlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth a quick comment on what drove the allowDiskUse change.

@Theodlz
Copy link
Collaborator Author

Theodlz commented Dec 13, 2023

Might be worth a quick comment on what drove the allowDiskUse change.

It actually shouldn't be necessary. Basically, before I noticed the missing stage (which I had on a local change, didn't commit it to the original PR I guess...) the sort stage wouldn't run because well it tried to grab the whole table. So I added the allowDiskUse that basically means that if you need more than the 100MB limit for an aggregation pipeline stage, you can use some of the disk.

Now that the missing stage is added it shouldn't be necessary. However, I will still leave it so that if in the future some objects accumulate enough data to go over that limit, it won't crash like it did.

This is already documented in multiple places in the code, so I'd rather not add that here (especially given that I am not sure it will ever be needed in this context now that the pipeline has been fixed).

@mcoughlin
Copy link
Collaborator

Sounds good Theo

@Theodlz Theodlz merged commit dc1a9bd into skyportal:main Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants