reindex when datasetUpdateRequired==false#6094
Conversation
and no Commands (which would trigger reindexing) are called.
| } | ||
| } | ||
| //We also need to reindex since no commands that include reindexing have been called. | ||
| indexService.indexDataset(dataset, true); |
There was a problem hiding this comment.
To fix the bug in 4.15.1, I could have added this at line ~1430, but reindexing is also needed at ~1438 for another case (see issue). I think adding it here catches both cases and avoids reindexing when it isn't needed (as would be the case if 1438 were uncommented).
I also tried to mimic the new exception handling/logging from #4425.
There was a problem hiding this comment.
So, really this logic should go through a Command. Not sure how it got in without that. We'll need to decide if we want his simple fix or require the refactoring to a command.
(my guess is we'll seetle on accepting this but open up a new issue for the refactoring - being in a command is important for permissions, automatic logging, and allowing same code for UI and API.
There was a problem hiding this comment.
+1 - it might be as easy as always forcing datasetUpdateRequired == true as was done with prov around line 1178 (or just removing that flag completely).
|
Can one of the admins verify this patch? |
|
@scolapasta, @qqmyers - can one of y'all refresh me on what we decided to do with this one? Has this been superseded by #6099 or is it different and should we still plan to move this along the board and get it merged? |
|
This is independent of #6099. There's one issue that was fixed in #6096 that will be obsoleted by #6099. This PR deals with a recent change related to onSuccess that will cause/reintroduce the same symptoms (making changes to files via the file page won't reindex and therefore won't update the search facets) in 4.16. This PR fixes that. The discussion is about whether there are better ways to do that, and when. It may be simple enough to remove the whole path that avoids using commands (which is the problem path) this week to hit 4.16 but it's definitely a bigger change than what's in this PR to avoid introducing a bug with 4.16. Your collective call, but I'd suggest merging this and adding removal of the datasetUpdateRequired==false option in EditDatafilesPage as a new issue. |
|
It's been too long, but I think with #6248, this became obsolete, so I'm closing it. |
5 participants
and no Commands (which would trigger reindexing) are called.
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
File metadata updates not reindexed #6092 File metadata updates not reindexed
Pull Request Checklist