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

Increase solr page size for csv download. #4246

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Conversation

justinlittman
Copy link
Contributor

refs #4245

Why was this change made?

Speedier reports

How was this change tested?

stage, prod

@justinlittman justinlittman force-pushed the t4245-large_reports branch 8 times, most recently from 62d2d21 to cb25406 Compare October 19, 2023 17:38
@justinlittman justinlittman marked this pull request as ready for review October 19, 2023 18:34
@@ -20,6 +20,7 @@ class << self

COLUMN_SELECTOR_COLUMN_SIZE = 3 # Helps display report columns in neatly lined up columns.
ROWS_PER_PAGE = 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we are only getting back from Solr the specific fields needed for the report, this could be bumped up to 200 or 250 for even better performance for large reports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, this is the number of results when a user scrolls through the report in Argo. A larger number might make this laggier since a larger response is going to be slower (though they could scroll through more results before a new page is requested). Since there hasn't been a request to change the report in Argo and I don't know why this number was selected, I'm not inclined to make a change at this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjgiarlo looks like this is your code. Feel free to chime in if you feel this is worth further consideration.

Copy link
Member

@mjgiarlo mjgiarlo Oct 19, 2023

Choose a reason for hiding this comment

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

You're correct, @justinlittman. I'm not super inclined to tweak the UI at this point since folks are happy with it.

Are your changes deployed anywhere? I'd be happy to do a quick test of the UI view to ensure the 10K change doesn't affect it, unless you've already done this.

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 just checked (in prod). The Report UI only pulls back 100 records.

Copy link
Member

Choose a reason for hiding this comment

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

Super, thank you!

@justinlittman justinlittman merged commit a23d2a1 into main Oct 19, 2023
@justinlittman justinlittman deleted the t4245-large_reports branch October 19, 2023 19:11
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.

3 participants