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

9635 solr improvements #10050

Merged
merged 30 commits into from
Nov 29, 2023
Merged

9635 solr improvements #10050

merged 30 commits into from
Nov 29, 2023

Conversation

landreev
Copy link
Contributor

@landreev landreev commented Oct 26, 2023

What this PR does / why we need it:

The following application-side improvements/workarounds are being added as part of this PR:

  • Adding app-side support for the Solr-side circuit breakers. Enabling those makes Solr start dropping requests with 503s (instead of dying) under heavy load. As currently implemented however, the page does not detect that condition and doesn't handle it in any special way, and simply shows zero search results/empty collection page. It needs to show something intelligent, along the lines of "search engine is experiencing heavy load, please try again later".
  • Optimizing the second search that the page repeats solely for the purpose of populating the type counts facets for the unchecked types. There is absolutely no need to repeat the same exact search, with all its super expensive parts, like all the other facets. There's no need to search for anything other than these one or two total counts; or to even run this second search, if all 3 type facets happen to be checked.
  • Adding nofollow to all the facet links and such, that we don't want crawlers/bots to touch. This will obviously only help with the nicer bots that take hints and follow rules, but better than nothing.
  • An off switch for facets support on the dataverse page. Again, facets are super expensive; it should be far better to be able to display the page with just the search cards and "facets are temporarily unavailable because of load issues" in the left column, than to have an empty page/have a "solr down" error page/have the page load take an hour.

Which issue(s) this PR closes:

Closes #9635

Special notes for your reviewer:

Suggestions on how to test this:

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?:

Additional documentation:

@landreev landreev added this to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation Oct 26, 2023
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@landreev
Copy link
Contributor Author

I just added a "closes" tag for #9635 to the PR. Once the pr is merged, I want to hot-patch our prod. with the improvements here, and then close the issue, at least for now. As I said before, there's definitely more to research and investigate and I want to get back to it, but there may be more pressing things r/n.

@github-actions

This comment has been minimized.

@landreev
Copy link
Contributor Author

OK, one test is legitimately failing in this PR, will fix.

…reindexing the

parent dataverse when adding guestbooks, stops the update dataverse command from
unnecessarily reindexing the underlying datasets in some cases, makes that reindex
truly async. in the onSuccess part of the command. (#9635)

This comment has been minimized.

…minates reindexing the"

(some things got checked in by mistake)
This reverts commit 18cd587.
…rk: eliminates reindexing the parent dataverse when adding guestbooks, stops the update dataverse command from unnecessarily reindexing the underlying datasets in some cases, makes that reindex truly async. in the onSuccess part of the command. (#9635)

This comment has been minimized.

1 similar comment

This comment has been minimized.

if (settingsWrapper.isTrueForKey(SettingsServiceBean.Key.DisableSolrFacets, false)) {
return false;
}

Copy link
Member

Choose a reason for hiding this comment

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

re: the following code - is it worth it to query solr just to see if you can query solr? Would a check of the last indexed time be a reasonable alternative? (Not sure how much this would save given it's a simple query).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this looks like it's yet another thing we can save/economize on.
We are already checking that this is the latest released version... so, yes, comparing the version published date against the indexed time stamp should be reliable enough.
These queries that just search for the version id cannot be that expensive; but there's definitely a lot of them in the logs.

guestbook.setEnabled(true);
dataverse.getGuestbooks().add(guestbook);
cmd = new UpdateDataverseCommand(dataverse, null, null, dvRequestService.getDataverseRequest(), null);
commandEngine.submit(cmd);
logger.info("Returned from command");
Copy link
Member

Choose a reason for hiding this comment

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

I added it just for debugging the async part - probably not needed, but at least should be fine()

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 actually noticed it, and meant to drop it. 😄

Copy link
Member

@qqmyers qqmyers 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. I haven't tested everything, but I have cherry-picked the nofollow links to QDR (and have the changes to the Guestbook page running there as well. The rest of the changes make sense.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Ready for Review ⏩ to Ready for QA ⏩ Nov 3, 2023
@landreev
Copy link
Contributor Author

landreev commented Nov 3, 2023

Looks good. I haven't tested everything, but I have cherry-picked the nofollow links to QDR (and have the changes to the Guestbook page running there as well. The rest of the changes make sense.

Cool, thank you. I will refactor the "version searchable" test above, and drop the .info log line quickly.

This comment has been minimized.

1 similar comment

This comment has been minimized.

@sekmiller
Copy link
Contributor

Seeing some weird results on the counts for dvobject facets in child Dataverses when those types are unselected.
Screen Shot 2023-11-15 at 10 42 07 AM
Screen Shot 2023-11-15 at 10 41 32 AM

@sekmiller
Copy link
Contributor

You can see the above on Dataverse internal at https://dataverse-internal.iq.harvard.edu/dataverse/sek11-15

(I added a couple of objects while facets were unavailable and it worked fine)

@landreev
Copy link
Contributor Author

@sekmiller
Just to confirm that yes, this is clearly a bug. The counts shown for the unchecked types are the totals for the root collection, not the current sub-collection.
... But I'm still working on a fix.

This comment has been minimized.

@landreev
Copy link
Contributor Author

@sekmiller
Sorry for the delay. The fix was quite simple actually.

@landreev landreev removed their assignment Nov 20, 2023

This comment has been minimized.

@pdurbin
Copy link
Member

pdurbin commented Nov 20, 2023

@sekmiller and I talked and I just kicked this off for a second opinion on API tests: https://github.com/gdcc/api-test-runner/actions/runs/6935079103

Update: it failed with Error: DataversesIT.testMalformedFacetQueryString:330 expected: <true> but was: <false>

@landreev
Copy link
Contributor Author

I still need to fix the API test above.

@landreev landreev self-assigned this Nov 27, 2023
… ways to improve detecting bad facet queries early on (and trying to decide if it's out of scope or not). #9635

This comment has been minimized.

Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:9635-solr-search-improvements
ghcr.io/gdcc/configbaker:9635-solr-search-improvements

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@landreev
Copy link
Contributor Author

The tests are passing now.

@cmbz cmbz added the Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) label Nov 29, 2023
@sekmiller sekmiller merged commit 960751f into develop Nov 29, 2023
24 checks passed
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Nov 29, 2023
@sekmiller sekmiller deleted the 9635-solr-search-improvements branch November 29, 2023 20:16
@pdurbin pdurbin added this to the 6.1 milestone Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 30 A percentage of a sprint. 21 hours. (formerly size:33)
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Investigate solr performance, diagnose bottlenecks and develop optimizations
6 participants