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

Add remoterepo query param #7580

Merged
merged 2 commits into from Oct 21, 2020
Merged

Add remoterepo query param #7580

merged 2 commits into from Oct 21, 2020

Conversation

agjohnson
Copy link
Contributor

This is used by new theme to perform search as you type directly against
the api response. API v2 is used here as we don't have this modeled at
all in APIv3 yet.

This is used by new theme to perform search as you type directly against
the api response. API v2 is used here as we don't have this modeled at
all in APIv3 yet.
@agjohnson agjohnson requested a review from a team October 19, 2020 18:32
@agjohnson agjohnson added the Improvement Minor improvement to code label Oct 19, 2020
stsewd
stsewd approved these changes Oct 19, 2020
readthedocs/api/v2/views/model_views.py Outdated Show resolved Hide resolved
Co-authored-by: Santos Gallegos <santos_g@outlook.com>
@@ -380,6 +380,9 @@ class RemoteRepositoryViewSet(viewsets.ReadOnlyModelViewSet):

def get_queryset(self):
query = self.model.objects.api(self.request.user)
full_name = self.request.query_params.get('full_name')
if full_name is not None:
query = query.filter(full_name__icontains=full_name)
Copy link
Member

Choose a reason for hiding this comment

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

This query could be super slow. This table is huge.

It seems that we are filtering by user first, and some month ago we created an index on full_name but it would be good to check that this won't take the db down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I'm not sure of the best way to test this though, other than looking at sql explain output or testing this in production. If you have any ideas to gauge this, it should be easy enough to run some queries directly.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't have a great idea about how to test this. However, a couple of queries on production could give us a good idea about how it will behave.

Also, I think we could enable "search as you type" under a feature flag (as a middle step for now) and if not enabled, it only filter by exact matches. Once we are sure that this is not taking down our DB, we can enable the feature to all the projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, it's integral to the design and is not optional. A feature flag won't work in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be fine shipping this to test the performance, and we can roll back if query times look awful on user data. I feel like NR will give us the best data here.

It's also probably only going to be an issue at scale or with high throughput. It's not documented either, so stakes are pretty low for at least a test.

Seem reasonable?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds risky, 😄 --but we should be fine, or we rollback otherwise. It would be good to have a way to enable/disable this without re-deploying, tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make a release note to test this more and watch NR, but i don't expect an issue. Manual queries in prod looked fine. It would be nice to have customer level feature flags, but honestly not much we'd use them for.

If APIv3 affords us any additional features for rate limiting, or token auth provides us with some additional security, we will eventually need this endpoint in APIv3 as well.

@agjohnson agjohnson merged commit 50f0fd0 into master Oct 21, 2020
2 checks passed
@agjohnson agjohnson deleted the agj/add-remoterepo-query-param branch October 21, 2020 18:05
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

Successfully merging this pull request may close these issues.

None yet

3 participants