search

tl-its-umich-edu / canvas-course-manager-next

Canvas Course Manager Next: A redesign of the existing CCM application. It extends Canvas features, makes cumbersome features easier to use, and adds new features.
8 stars 9 forks source link

Handle too many matching course results (#378) #379

Closed ssciolla closed 2 years ago

ssciolla commented 2 years ago

The PR aims to resolve #378. It also fixes a logging miscalculation introduced in #375 and adds an isInteger value check to the consumption of a few configuration variables, including the new CANVAS_MAX_SEARCH_COURSES.

ssciolla commented 2 years ago

@pushyamig, I'm still doing testing and looking into this more, but I thought I'd get the review process started.

pushyamig commented 2 years ago

Have you decide how many threads and mem limit we will be configuring in non-prod/prod? or we are still doing testing around that decision?

ssciolla commented 2 years ago

Have you decide how many threads and mem limit we will be configuring in non-prod/prod? or we are still doing testing around that decision?

I think we're still doing testing, but I thought we decided a change like this was also probably necessary so people don't sit there waiting forever for large queries.

pushyamig commented 2 years ago

I will test this in afternoon

pushyamig commented 2 years ago

@ssciolla this seems to be working. I added this PR to ccm-dev. This look ok to me from what I could test.

pushyamig commented 2 years ago

Why is this still a draft? This is functional as far as I tested and reviewing the code.

ssciolla commented 2 years ago

Okay, @pushyamig, I opened it for full review.