VUFIND-1342 Modernize browse indexer #48
Conversation
There was a problem hiding this comment.
Thanks, @marktriggs, this looks great! The test suite is passing, so I've gone ahead and pushed the binaries into vufind-org/vufind#3253 so it is ready to merge when this is finished. (And of course I'll update them if other feedback leads to changes in the meantime... but since I had to put them there to test, I figured I might as well commit in case it saves us time in future).
See below for one tiny documentation suggestion, and an observation of no great importance.
There was a problem hiding this comment.
Thanks, @marktriggs! I'm approving this, but I'll hold off on merging it for a few days in case @todolson or others have any additional comments.
There was a problem hiding this comment.
I have not fully (re?)digested PrintBrowseHeadings.java, but so far I do think this all looks good. I see no reason to stand in the way.
Thank you, @marktriggs, for taking on this overdue modernization!
| /** | ||
| * Load headings from the index into a file. | ||
| * | ||
| * @param fieldIterator Leech for pulling in headings |
There was a problem hiding this comment.
Late comment: would be good to change this parameter description, since the Leech classes have all been renamed. Maybe something like:
"Iterator for pulling in headings from Solr matches"
| throws Exception | ||
| { | ||
| BrowseEntry h; | ||
| while ((h = fieldIterator.next()) != null) { |
There was a problem hiding this comment.
This is probably out of scope for the current effort, but this makes me wonder if SolrFieldIterator should implement the Iterator interface sometime in the future. It already has a next() method, so it's mostly there.
There was a problem hiding this comment.
I agree with @todolson that it's probably a good idea to change the comment(s) that use the word "Leech" for consistency, but more substantive changes can be addressed separately. Do you mind making the edit, @marktriggs, or would you like me to take care of it for you?
I can pick that up since I'm still set up for testing, thanks for the review @todolson. I'll look at adding the Iterator interface too; might as well :) |
|
Thanks! I'll wait for this to get finished before I take on #49, though it looks like that's going to be an easy one to merge. :-) |
|
@demiankatz @todolson I think that's ready to go now. I ended up implementing both |
* Move all files to sit under packages:
- org.vufind.util
- org.vufind.solr.handler
- org.vufind.solr.indexing
* Remove unused clojure jar (a previous test suite used this, but
it's gone)
* Remove unused build targets from build.xml
These work the way they used to, but log warning messages
Also modifies PrintBrowseHeadings to take advantage of the Iterable.
demiankatz
force-pushed
the
VUFIND-1342
branch
from
d5d1b39 to
57e587e
Compare
This PR addresses the issues described here:
https://openlibraryfoundation.atlassian.net/browse/VUFIND-1342
I've finished my first pass across everything, and thought I'd get a PR up to invite feedback. The story so far:
The unqualified classes used for indexing are now in the
org.vufind.solr.indexingpackageThe word "leech" has been replaced with "FieldIterator" in the places it appeared
All FieldIterators are autoclosable, and we take advantage of that where possible
Cleaned up imports (expanded wildcards and removed unused imports)
To preserve compatibility with existing usage, there are wrapper versions of
CreateBrowseSQLiteandPrintBrowseHeadingsunder acompatdirectory that log deprecation warnings and then pass through to the new versions. There's also code to detect usage of old environment variables and system properties (BIBLEECH) and old versions of class names. All of this code can probably be thrown out in a future release, once people have had time to update their scripts.Besides that, tests are all still passing, and I have a corresponding PR against VuFind to update the
index-alphabetic-browsescripts to use the new names. I'll link to this PR from there once I send it.TODO