tumblr / collins

groovy kind of love
tumblr.github.com/collins
Apache License 2.0
571 stars 99 forks source link

Fix multi-collins queries #407

Closed defect closed 8 years ago

defect commented 8 years ago

This is a follow-up to #396 and tries to fix a couple of issues with multi-collins queries.

1. Send CQL query to remote instances (848a11a)

This is what #396 tried to fix but didn't since I actually sent the query in Solr format instead of CQL to the remote instances. To fix this, I've changed the way we handle CQL queries by storing them as a String instead of a SolrExpression, and only parse the query to solr syntax when we're actually planning to execute it. This means that we can pass the raw CQL query string to the remote instances.

2. Fix parsing of remote responses where state is null (04571fd)

Previously, if an asset a remote instance returned didn't have a state, the JSON parser would fail on the null value.

3. Fix filtering of the local instance (9025bcd)

When doing a multi-collins query we try to filter out the local instance (the one we actually send the query to) based on the multicollins.thisInstance config variable. However, we're comparing a String with an Option[String] which fails so we'll actually get the results from the local instance twice, once for the multi-collins query and once for the local.

@tumblr/collins @tbchrist @bobpattersonjr

CC'ing you too @cburroughs, since it would be interesting to hear feedback based on your multi-collins use cases.

roymarantz commented 8 years ago

:+1: if this works ,since it is hard to argue with success :-) , even with my comments

william-richard commented 8 years ago

LGTM