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

Force async processing for large data sources on restricted domains #35465

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mkangia
Copy link
Contributor

@mkangia mkangia commented Dec 3, 2024

Product Description

When saving a large data source (current default threshold is 1M), users will see the following error message to enforce asynchronous processing

image

Additionally, Minor improvement to show an error message on top if save fails for a data source configuration

image

Technical Summary

https://dimagi.atlassian.net/browse/SC-4001

Reused the existing feature flag that limits data source rebuilds to enforce asynchronous processing & slightly modifying the existing restriction by that feature flag to apply only for synchronous data sources.

Feature Flag

RESTRICT_DATA_SOURCE_REBUILD

Safety Assurance

Safety story

Tested locally.

Automated test coverage

QA Plan

https://dimagi.atlassian.net/browse/QA-7317

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@mkangia mkangia marked this pull request as ready for review December 3, 2024 15:24
@mkangia mkangia requested a review from calellowitz as a code owner December 3, 2024 15:24
@mkangia mkangia added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Dec 3, 2024
@mkangia mkangia force-pushed the mk/4001-force-async-processing branch from a91feee to cc37c7b Compare December 3, 2024 21:49
@mkangia mkangia force-pushed the mk/4001-force-async-processing branch from cc37c7b to e279bc0 Compare December 3, 2024 21:54
@Charl1996
Copy link
Contributor

@mkangia

Why don't we mark it automatically for the user if know the data source covers a lot of data? It doesn't seem like a good UX when we only tell a user that they should have done something after they've tried to do it, especially if we can know that beforehand.

Any thoughts?

@mkangia
Copy link
Contributor Author

mkangia commented Dec 4, 2024

Hey @Charl1996

Great points.

Couple of things that I considered:

  1. I was hesitant to process the data source asynchronously if it was not marked to do so
  2. I was hesitant to set something on the data source, which was created by the user & that too without the user knowing about it. We could else then mark it as async but also notify user we did so, along with explanation, which seemed to get too stretched for the implementation. So, I chose to rather tell the user to do so, which has an additional benefit of user getting aware of this feature and using it more actively in future.

Happy to reconsider if you think we are adding unnecessary step for the user. I was just thinking of being more explicit.

@Charl1996
Copy link
Contributor

Happy to reconsider if you think we are adding unnecessary step for the user

I don't have too strong feelings either way, so fine to go with the way you have it. It does have the benefit of being more explicit and making the user aware of the setting.

Copy link
Contributor

@zandre-eng zandre-eng left a comment

Choose a reason for hiding this comment

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

Looks good. Just some minor nits.

corehq/apps/userreports/ui/forms.py Outdated Show resolved Hide resolved
@@ -1216,6 +1216,8 @@ def post(self, request, *args, **kwargs):
return HttpResponseRedirect(reverse(
EditDataSourceView.urlname, args=[self.domain, config._id])
)
else:
messages.error(request, _('Data source not saved!'))
except BadSpecError as e:
messages.error(request, str(e))
Copy link
Contributor

@zandre-eng zandre-eng Dec 13, 2024

Choose a reason for hiding this comment

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

This isn't part of your changes so feel free to ignore, but it seems we are returning an exception message back to the user here. Might be good to confirm that this is simply returning a general message and not any sort of stack trace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled QA Passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants