Add option of terminate_after when run a query #79
Conversation
|
@rcoh PR for enabling terminate_after when running a query. |
Current coverage is 61.04% (diff: 73.33%)@@ master #79 diff @@
==========================================
Files 14 14
Lines 906 919 +13
Methods 0 0
Messages 0 0
Branches 228 232 +4
==========================================
+ Hits 552 561 +9
- Misses 210 211 +1
- Partials 144 147 +3
|
|
|
||
| // This api should be incorporated into QueryRoot in 2.0 when breaking changes are allowed. | ||
| // Before that, this api can serve to extend QueryRoot with backward compatibility. | ||
| class ExtendedQueryRoot(override val query: Query, |
There was a problem hiding this comment.
I must be missing something. Why can't we just add terminateAfter with a default value and keep backwards compat?
There was a problem hiding this comment.
We have QueryRoot object which has an apply method that has parameters with default values. QueryRoot case class also has an apply method.
Currently, all QueryRoot case class's parameters do not have default values, which is fine.
However, if we were to add terminateAfter: Option[Int]=None, the code will not compile due to scala compiler disallowing overloaded methods with default arguments.
There was a problem hiding this comment.
There was a problem hiding this comment.
oh right! Forgot about that bit of nastiness. If you want, you could add another optional parameter to the apply method. If terminateAfter was defined, we'd return an ExtendedQueryRoot instead. That way we hide the problem inside of the library. What do you think?
There was a problem hiding this comment.
That makes sense. Let me put in that.
|
OK LGTM. I made a suggestion above you can take if you want. Seems like we're building up enough stuff to cut 2.0 soon. |
| val sorts = sortOpt | ||
| .map(_.foldLeft(Seq.empty[Sort])((sorts, value) => sorts :+ SimpleSort(value._1, SortOrder.fromString(value._2)))) | ||
| .getOrElse(Nil) | ||
| new QueryRoot(query, fromOpt, sizeOpt, sorts, timeout, sourceFilter) | ||
| if (terminateAfter.isDefined) { |
There was a problem hiding this comment.
terminateAfter
.map(tA => new Ex....)
.getOrElse(new Query Root....)
It turns out terminate_after can have huge impact on latency when the document counts are large.
In order to support terminate_after and maintain backward compatibility, I have to create an ExtendedQueryRoot that inherits QueryRoot. This api can serve to extend any further enhancements to QueryRoot before 2.0.
This api should be incorporated into QueryRoot in 2.0 when breaking changes are allowed.