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

Fix for bug 20 - luceneMatchVersion #21

Merged
merged 2 commits into from
Jun 7, 2013

Conversation

janhoy
Copy link
Contributor

@janhoy janhoy commented Jun 2, 2013

This avoids luceneMatchVersion in config!
Bumps to v1.3.1

@janhoy
Copy link
Contributor Author

janhoy commented Jun 2, 2013

After pull #19 and this patch, I still have a few improvement suggestsions:

  • We now only use the TokenFilterFactory.forName() (or TokenizerFactory.forName()) method of finding classes. However, the config should not use the <str name="class">...</str> property for that, but a new <str name="name">...</str>, since we're looking up the logical name, not the class name
  • We should then bring back the ability of loading tokenizers and filters by class name from the class config property, if it exists. Either through ResourceLoader or investigate the Token*Factory.lookupClass() method.

An alterntative strategy is of course to use the class property for both - first trying forName and if it fails, then try by class name.

I do not have time to fix this right now, so feel free to add it in yourself

@nolanlawson
Copy link
Member

Looks good! I agree with you about the configuration. But pursuant to #10, I'm wondering if it wouldn't make more sense to just overhaul the configuration entirely. There's really no need for the user to modify fine-grained details in the ShingleFilterFactory or ``TokenFilterFactory` - those can be abstracted away (as you yourself suggested in the Solr bug, if I recall correctly).

Or are you now proposing that we keep the configuration largely as-is and just make that one small change?

nolanlawson added a commit that referenced this pull request Jun 7, 2013
Fix for bug 20 - luceneMatchVersion
@nolanlawson nolanlawson merged commit 422fd97 into healthonnet:master Jun 7, 2013
@janhoy
Copy link
Contributor Author

janhoy commented Jun 7, 2013

I thought I sent this comment earlier, but here we go:

I chose to use the class attribute both for loading by class name as before and by service name. It wil first try by service name, and fallback to class if it fails. In addition, it will print a stack trace if className does not contain "." and loading by service name still fails - this is to get the useful printout of supported service names for easier debugging.

The correct way of loading by resource loader was not Token*Factory.lookupClass but rather to use SolrResourceLoader's newInstance constructor with extra arguments for the params.

With the last commit, the config is again back-compat with earlier versions, since it will simply ignore any luceneMatchVersion. I consider v1.3.0 which is currently published to be buggy since it requires config change. It should be replaced with this 1.3.1 and not downloadable.

@janhoy janhoy deleted the luceneMatchVersion branch June 7, 2013 09:51
@janhoy
Copy link
Contributor Author

janhoy commented Jun 7, 2013

Or are you now proposing that we keep the configuration largely as-is and just make that one small change?

No, I still think #10 makes sense, it was just that I needed a version working with 4.3 so I did the quickest fix for it. Have not started investigating how much work is involved in #10

@nolanlawson
Copy link
Member

Awesome work, Jan. We'll get to #10 eventually; for now, though, this really streamlines the config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants