search

collective / collective.recipe.solrinstance

Buildout recipe to configure a Solr instance
https://pypi.python.org/pypi/collective.recipe.solrinstance
5 stars 13 forks source link

Added - ability to configure remote streaming #30

Closed wengole closed 10 years ago

davidjb commented 10 years ago

With this change, the option for Solr 4 templates is now defaulting to false rather than true, as it currently is. By contrast, I note that the older Solr templates have false as their current default. I've not used this feature myself, so can't comment on what effect this has or what is otherwise expected. Anyone else able to comment? If there's a suitable reason to default this feature to off, that's okay; I feel consistency is best.

In any case, can you please add a change note to CHANGES.rst as part of this feature? If the change in behaviour stands, then documenting this like other backwards incompatible changes is a must.

lukasgraf commented 10 years ago

I also haven't used this feature (knowingly). But I would rather not have the default configuration change unless there's a pressing reason. Since it would now be configurable, anyone that doesn't want the feature for Solr 4 setups can turn it off.

wengole commented 10 years ago

The Solr default is off and always has been as it's considered a security risk to have it on (anyone can push data into you solr instance using curl).

See https://wiki.apache.org/solr/ContentStream#RemoteStreaming

We required it to be set to true and were using a previous version which didn't support solr4. On upgrading we noticed that the old templates have it set to false (and unconfigurable) and the solr4 templates had been recently chaged to true. Although this is what we needed, we felt it would be better to adhere to the solr default and allow it to be configurable.

Solr 4 tempaltes

Old Solr templates

Commit where default was changed

davidjb commented 10 years ago

+1 for merging this. I'm unsure why the templates were recently changed by @giacomos but keeping the expected default behaviour is a good idea, especially given what this feature does. If anyone was relying on that change and needs this option, they can now enable it. Any objections?

lukasgraf commented 10 years ago

After that reasoning, I gotta say +1 for changing the default as well. :+1:

But please fix the failing tests before merging (resp. I'm not even sure if it's just a test failure, from a quick glance it seems like SolrBase.options_orig might need to be initialized).