Skip to content

Performance improvements for the Site Configurations when completion is enabled - #486

Merged
OmarIthawi merged 2 commits into
appsembler/tahoe/developfrom
omar/fix-giant-query-for-completion
Nov 25, 2019
Merged

Performance improvements for the Site Configurations when completion is enabled#486
OmarIthawi merged 2 commits into
appsembler/tahoe/developfrom
omar/fix-giant-query-for-completion

Conversation

@OmarIthawi

@OmarIthawi OmarIthawi commented Nov 21, 2019

Copy link
Copy Markdown

This pull request does two changes:

  • Improve the performance of retrieve_last_sitewide_block. By making the SELECT * FROM site_configuration_siteconfiguration query only when needed.
  • Make the URL returned from retrieve_last_sitewide_block site aware. By removing the domain part and using relative URLs instead i.e. starting with /.

@OmarIthawi OmarIthawi changed the title (WIP) Performance improvements for the Site Configurations when completion is enabled Performance improvements for the Site Configurations when completion is enabled Nov 21, 2019
@OmarIthawi
OmarIthawi force-pushed the omar/fix-giant-query-for-completion branch from 5c3a453 to ba2b92e Compare November 21, 2019 11:52
@OmarIthawi
OmarIthawi marked this pull request as ready for review November 21, 2019 11:55
@thraxil

thraxil commented Nov 21, 2019

Copy link
Copy Markdown

Are we positive that retrieve_last_sitewide_block only gets called in a web context, where we know a relative URL will work? Ie, if the URL it generates ends up in an email, a relative one without the host/site wouldn't work.

@melvinsoft melvinsoft Nov 21, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@OmarIthawi I'm a bit worried about how this approach affects enterprise or normal edX installations (if we want to push upstream) because is gonna pull SiteConfiguration.objects.all() on every course the user is enrolled in, every-time a user reach the dashboard.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good point, @melvinsoft . A couple of questions for @OmarIthawi and/or @melvinsoft

A. Does it make sense to add a tahoe/multisite env flag for this?
B. Where is the source of 'course_org_filter'? I've looked over the SiteConfiguration class and have trouble finding the source of truth for this field

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Great point @melvinsoft, I've missed that. I'll improve the task to be at least equal to the original behaviour, not worse.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks @johnbaldwin, I'll reply inline:

A. Does it make sense to add a tahoe/multisite env flag for this?

Maybe but I'm trying to avoid having such configuration to avoid having too much conflicts when we merge with Open edX.

B. Where is the source of 'course_org_filter'? I've looked over the SiteConfiguration class and have trouble finding the source of truth for this field

That would be found in the SiteConfiguration.values dictionary (example):

image

@melvinsoft

Copy link
Copy Markdown

@OmarIthawi Thanks for putting this together, I just posted a question for you.

By removing the `domain` part and using relative URLs instead
@OmarIthawi
OmarIthawi force-pushed the omar/fix-giant-query-for-completion branch from ba2b92e to 8e02399 Compare November 24, 2019 00:03
@OmarIthawi

Copy link
Copy Markdown
Author

@melvinsoft I think I've fixed the issue, please take a look.

@OmarIthawi
OmarIthawi force-pushed the omar/fix-giant-query-for-completion branch from 8e02399 to 1f1d8d5 Compare November 24, 2019 00:13
By making the query only when needed
@OmarIthawi
OmarIthawi force-pushed the omar/fix-giant-query-for-completion branch from 1f1d8d5 to 3952a04 Compare November 24, 2019 04:10
@OmarIthawi

Copy link
Copy Markdown
Author

@thraxil

Are we positive that retrieve_last_sitewide_block only gets called in a web context, where we know a relative URL will work? Ie, if the URL it generates ends up in an email, a relative one without the host/site wouldn't work.

I have checked it and the function doesn't seem to be used anywhere other than web requests.

@melvinsoft

I'm a bit worried about how this approach affects enterprise or normal edX installations (if we want to push upstream) because is gonna pull SiteConfiguration.objects.all() on every course the user is enrolled in, every-time a user reach the dashboard.

I've fixed this and the query now runs one time at max, and only when needed.

@OmarIthawi
OmarIthawi requested a review from thraxil November 25, 2019 07:02
@OmarIthawi
OmarIthawi merged commit 6d29ef2 into appsembler/tahoe/develop Nov 25, 2019
@OmarIthawi
OmarIthawi deleted the omar/fix-giant-query-for-completion branch November 25, 2019 11:43
@melvinsoft

Copy link
Copy Markdown

@OmarIthawi Thanks for the fix!

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.

4 participants