ISLANDORA-1392: New Islandora Solr Views mechanic #39
Conversation
Jira: [ISLANDORA-1392](https://jira.duraspace.org/browse/ISLANDORA-1392) and some other un-detected stuff needed to fix this. Also: https://groups.google.com/forum/#!searchin/islandora/views/islandora/LVz 0-JF9fxU/o4jOKV6FAgAJ # What does this Pull Request do? This "Huge" pull request changes the way Islandora Solr Views defines it's fields and handlers to fix freezing and out of memory issues with Solr Indexes that have a large Solr field count. It also fixes a lot of small bugs and adds some additional functionality to this custom view module. # What's new? Instead of having a particular views field matching every existing Solr fields, we define a discrete set of fields classified by type: * General Solr field * A multipurpose views field that allows to use any existing solr field for displaying, filtering and context exposing * Sortable Solr field * A Sortable Solr field that allows a view to be sorted and also can be exposed * Date Solr field * A Solr Date field * Score * A simple score field to sort/display Each of this fields has an option to define to which real-exisiting solr field to "bind", which can be done through an autocomplete form element that only shows those Solr field that match the type (e.g, Date Solr field only shows Solr fields of type 'date'; Sort only sortable, non multivalued ones). Additionally, it's possible to disable facet calculation at all to make view faster. This is done in `'Other' -> 'Query settings' -> 'settings'` during View editing. If facet is `enabled` (`disabled `by default), then the global Islandora Solr Query processor variable is also defined, allowing to use blocks that depend in this one. Lastly a new options is available for Filter fields. There is a `'Don't escape filter value for Solr'` option. When enabled, the Filter value set will not escaped (dangerous) but will allow to use filters like `[ * TO NOW]` and other non-literal strings, functionality not previously available. Please don't expose this ones to user interaction! I'm pretty sure i have put a function to check for this… # How should this be tested? Prior to testing I encourage people to export/backup their current Islandora Solr views. They won't work anymore and will need some refactoring. Once enabled, as any other existing View. Choose which Fields, Filters and Sort to use from the limited set. On each add/edit form you have the option to select which real Solr field you want to select from the index. It defaults always to 'PID' # Backwards compatibility: Sadly it's not backwards compatible with previous Islandora Solr Views, which basically means you have to make new views from scratch, or export your current ones and edit the values to match the new structure. # Additional Notes: This is a big pull, was tested and re-tested but can have some issues, so please comment/interact with me so we can detect and correct them. Could this change impact execution of existing code? Yes, of course, it will. Since the whole mechanic changed no previous Views build using this module will work out of the box. Additionally, some code clean up (coding standards) was made. Sadly it was impossible to fix every already existing issue. more over because Drupal's Views module does not respect coding standards used by us. Also, since we can now put a facet block next to a Islandora Solr View, there is a need to make a new block that is able to handle view interaction. The current one has the nasty habit to use the current path for links, making a click on a facet have no good consequences at all! Work is on progress to make a views respectful facet block that allows to interact with the current view. --- Thanks a lot! Diego Pino Navarro A dog & human friendly developer
Fixed a coding standard error
Now properly fixed (i hope)
Copy/paste detector was having trouble with similar multiple handlers. Not sure if having copy/pasta check is good since not even Drupal Views modules passes that one. But ok, i moved things around. lets hope nothing got broken!
|
Tagging @jordandukart and @adam-vessey or any other @Islandora/7-x-1-x-committers who is interested on this, since it's been 2 months since the first pull. Thanks! |
|
@qadan should we prioritize review on for the code freeze? |
|
@DiegoPino @qadan I can't 100% recall what the consensus was on this; I recall some discussion being had in committers at some point regarding the backwards-incompatibility of this and whether it would be appropriate to write an update hook to port views to the new method, or what other solution was appropriate, but I don't recall the outcome of that discussion. It'd be my opinion that we hold off on this until we have a clear upgrade path for users with Solr views that are moving to 7.x-1.7 (or whatever), as this could be something of an upgrade disincentive to existing users with many and/or complex existing views, but I'd rather get thoughts on that (or recall the ones already given haha ...) |
|
Hi @qadan and @ruebot . I'm fully Ok with doing some sort of migration mechanic, even when it may involve a lot of coding, but i would like some review on the pull itself as well. Basically because adding that code is a lot of extra effort (i'm willing to make) but does no sense if people, committers, etc are not OK with the changes i did, nor haven't tested the code at all (which can have issues of course as any contributed code). Thanks! |
|
Hey @DiegoPino, I'll take a look at this for you soon if we need motion on this. |
|
@daniel-dgi, thank you very much. |
|
Since @daniel-dgi is away, how do @Islandora/7-x-1-x-committers want to proceed with this? |
|
@ruebot i was asked in the forums to build a views migrator. Will give that a last try. If no reviewafter that i will move this module to a standalone alternative for people that need this fixed, at least in a parallel implementation. Thanks. |
|
Think this has been discussed on numerous committers calls, stated in the PR template and touched on in https://jira.duraspace.org/browse/ISLANDORA-1293?focusedCommentId=49497&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-49497, I believe the original concern about backwards breaking compatibility still exists. |
|
@DiegoPino Is there any chance of seeing some movement on this PR, or has the community decided not to go with it? |
|
@bondjimbond has not been decided. People are actually using this pull in production quite a lot. Others don't. It does break existing Views but I also don't have time (really no time, not an excuse) to make a views migration system right now. |
|
@bondjimbond in your opinion, what should be done? |
|
I'm generally in favour of making something broken work, even if it means that the things built with the broken tool need fixing because of it. But this is a tough one. Do we have any notion of the extent of the work required for an Islandora user/administrator to make old views work with this new version? Like, at which points exactly does this break them? |
|
This pull changes the "key" to a the field definition is set, basically splitting the fields into categories. Instead of having N number of views fields == number of Solr fields, you always have the same amount, like 6? And each "type", date, sortable, etc, has the actual field name (homolog to the solr field name) as a parameter. This adds an additional dimension to the export and import of course (its an array). But solves the too many fields issue completely: also there are quite other improvements here. |
|
@bondjimbond I guess if I get some help I could eventually make it to the release by adding more logic. The issue is my pull requests (and in general many from other people) stay without testers for too long. I do what I can, but We need more community commitment on that for sure. Will plan for some updates and see how can that be done. |
|
@DiegoPino If you can suggest a few of the kinds of views that might be impacted by this (maybe the built-in ones, perhaps an export of a more complex view), I could test next week (I'm on vacation now till Tuesday) and offer some feedback. From the commentary and my recollection of various calls, I'm not sure if this saw much testing or if the concerns were mostly speculation. I'd really love to see this pull happen, 'cause the problems with Solr Views are crippling. |
|
@DiegoPino Coming back to this now since I've got so many problems with my Solr Views... Is this pull request dead in the water, or does it have a chance? If you can provide some examples of changes one would need to make to update a view for this pull, I can help with testing. |
|
In production on this side and working but I can promote this into a proper merge with a little of work and your will to review. If you can remind me this week (friday?) i can revive it, have so many pulls in the waiting. (everyday a new need right!) |
|
So, @DiegoPino -- any chance you'll have time to work on this at some point? (Apparently I needed a reminder to remind you.) Given that we're in code freeze I expect it'll be a while, but want to keep this on your radar. I'll be very happy to review when you've got it ready. |
|
Yes. I will be able. Are you not in the release call??? |
|
Also, thanks for reminding me! |
|
Egad, is that now? I didn't see the notification! Whups. :/ |
|
@bondjimbond every 2 weeks until it is released. So we see you april 26th? 😆 |
|
Yep... I forgot to set it to "repeat" in my calendar. Caught the very end of this one... I'll be there for the next one. |
|
@DiegoPino Do we have any process on this? |
|
Yes, I would love to see this happen. I will be very happy to test if @DiegoPino wants to update the pull. |
|
@DiegoPino Any chance this PR could happen before code freeze? I'd love to see it arrive in time for release. |
|
@bondjimbond issue i have is the following: To make this happen with consensus (other than rebasing because this all works since its conception 2-3 years ago?) i would have to basically write a smart routine that re-creates all existing Solr based Views people have. Means 200% chances of failure because how do i know they did them right? Or Edge cases. This is if you all need to maintain backwards compatibility... Code freeze is October 19, and there is also DLF2018.. same week. So yeah, -maybe i could- but who would test? This requires a bunch of devoted testers. If you have some of them around! then OK. Side note. I see the issue this one fixes as a BUG not an improvement. If a normal Islandora Solr index breaks this one, then well, its a bug. So under those circumstances we could-maybe have more time for testing? |
|
@bryjbrown Your thoughts on Diego's comments above? If this is a bug, then it's something we could be testing and working on during the code freeze.. |
|
@bondjimbond I see that the JIRA ticket this references is listed as a bug, so that would extend the timer for working on this past the code freeze date. Whether we can realistically get a nearly 3 year old PR squared away for this release is a different question. I'm not familiar with Islandora Solr Views so I'll defer to @jordandukart as component manager on how to proceed with this one. |
|
Given the nature of the changes that are happening here 100% backwards compatibility is paramount due to the widespread use of the current structure of the Solr Views handler. @DiegoPino has done a great job at addressing the underlying issues with the amount of fields that are created in the views_data hook. These changes clean up the UI and prevent memory exhaustion problems that can occur when there are a lot of unique Solr fields within in an index. In a perfect world, backwards compatibility can be achieved and this moves forward and gets merged. More realistically, given the amount of effort needed in both developing and testing this, this should live as a separate implementation of Solr views. Where/how that exists I'm not sure but those are my thoughts. |
|
Option 1: @jordandukart @bryjbrown @bondjimbond if you are all OK, I can create a second views query handler inside the same module here https://github.com/Islandora/islandora_solr_views/blob/7.x/islandora_solr_views.views.inc#L15-L16 Option 2 is to create a new module, self-managed, not under the Islandora jurisdiction that deals with the issue. But, again, the issue this one is trying to fix is a bug and last time I checked quite a few are running this pull in production. You decide, please let me know, I can work on Sundays on this stuff. Thanks! |
|
So @bryjbrown @bondjimbond. Did you come to an agreement on how I should proceed? Another Solr handler? Or make a new, no-Islandora managed module that people can use instead of this? Or other ideas? Let me know, maybe 3 years is enough and this can eventually be resolved =) Thanks! |
|
@DiegoPino I see no reason why the separate handler idea wouldn't work, that should allow existing code and views to use the older handler without breaking and give folks the option to migrate their code and views over to the new handler at their own convenience. This sounds like a better idea than having a separate module to me because if we have two separate modules, it forces any module that wants to use the new handler to change dependencies, and we may end up with sites that have both modules installed due to the different requirements of various other modules/solution packs. That's how it seems to me, but once again with the old caveat that I'm not familiar with Islandora Solr stuff so I'll defer to others who know more about it. My $0.02. |
|
@bryjbrown thanks, sounds good. Will give that a try. |
Jira:
ISLANDORA-1392 and
some other un-detected stuff needed to fix this and resolved as code tasks.
Also:
https://groups.google.com/forum/#!searchin/islandora/views/islandora/LVz0-JF9fxU/o4jOKV6FAgAJ
What does this Pull Request do?
This "Huge" pull request changes the way Islandora Solr Views defines it's fields and handlers to fix freezing and out of memory issues with Solr Indexes that have a large Solr field count. It also fixes a lot of small bugs and adds some additional functionality to this custom view module.
What's new?
Instead of having a particular views field matching every existing Solr fields, we define a discrete set of fields classified by type:
field for displaying, filtering and context exposing
be exposed
Each of this fields has an option to define to which real-existing solr field to "bind", which can be done through an autocomplete form element that only shows those Solr field that match the type (e.g, Date Solr field only shows Solr fields of type 'date'; Sort only sortable,
non multivalued indexed ones).
Additionally, it's possible to disable facet calculation at all to make
view faster. This is done in
'Other' -> 'Query settings' -> 'settings'during View editing.If facet is
enabled(disabledby default), then the global Islandora Solr Query processor variable is also defined, allowing to use blocks that depend in this one.Lastly a new options is available for Filter fields. There is a
'Don't escape filter value for Solr'option. When enabled, the Filter value set will not escaped (dangerous) but will allow to use filters like[* TO NOW]and other non-literal strings, functionality not previously available. Please don't expose this ones to user interaction!I'm pretty sure i have put a function to check for this…
How should this be tested?
Prior to testing I encourage people to export/backup their current Islandora Solr views. They won't work anymore and will need some refactoring. (Suggestion was made by @adam-vessey we could use an automated way in drupal to migrate, fully possible to make and a great suggestion).
Once enabled, as any other existing View. Choose which Fields, Filters and Sort to use from the limited set. On each add/edit form you have the option to select which real Solr field you want to select from the index. It defaults always to 'PID'
Backwards compatibility:
Sadly it's not backwards compatible with previous Islandora Solr Views, which basically means you have to make new views from scratch, or export your current ones and edit the values to match the new structure.
Additional Notes:
This is a big pull, was tested and re-tested but can have some issues, so please comment/interact with me so we can detect and correct them.
Could this change impact execution of existing code? Yes, of course, it will. Since the whole mechanic changed no previous Views build using this module will work out of the box.
Additionally, some code clean up (coding standards) was made. Sadly it was impossible to fix every already existing issue. More over because Drupal's Views module does not respect coding standards used by us.
Also, since we can now put a facet block next to a Islandora Solr View, there is a need to make a new block that is able to handle view interaction. The current one has the nasty habit to use the current path for links, making a click on a facet have no good consequences at all! Work is on progress to make a views respectful facet block that allows to interact with the current view.
Thanks a lot!
Diego Pino Navarro
A dog & human friendly developer