OpPivot;OpLimit - pivot not respecting limit?

[stewart.allen]

since the change to OpLimit where it no longer throws a runtime exception, I receive the following log lines with a pivot;limit chain

ERROR [queryOp-25] 2014-02-09 15:07:04,981 OpLimit.java (line 74) Limit reached, sendComplete was called.
ERROR [queryOp-25] 2014-02-09 15:07:04,981 OpLimit.java (line 74) Limit reached, sendComplete was called.
ERROR [queryOp-25] 2014-02-09 15:07:04,981 OpLimit.java (line 74) Limit reached, sendComplete was called.
ERROR [queryOp-25] 2014-02-09 15:07:04,981 OpLimit.java (line 74) Limit reached, sendComplete was called.

where that line is emitted for each line after the limit is reached. it may be the case only when a sort to disk threshold is reached.

[Matt Abrams]

Yes. the code used to break using the exception, obviously not a good way to control logic flow. For now we should remove the log line. Near future we need to blow up query observer and provide a non destructive way to exit the op early.

You can modify your log4j config to exclude log messages from this class.

[Matt Abrams]

In short term I think it is best to inject more QueryStatusObserver into Ops that send more than one bundle at a time. Pull request created: https://github.com/addthis/hydra/pull/18

[Ian Barfield]

Can you elaborate a bit on the relationship between these log lines (or the previous exceptions) and the disk sort op? The disk sort code seems to already obsessively check the status observer, and I was unable to produce any log lines by using various sort/limit topographies.

--
You received this message because you are subscribed to the Google Groups "hydra-oss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to hydra-oss+...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Matt Abrams]

The relationship is that any operation which produces bundles should
stop the chain once the query has been completed or it has been
canceled.

For example OpPivot invokes the 'sendTable' method which will send 1
or more bundles based on how many rows exist in the table. If the
next op is a limit op the scenario can occur where pivot produces
bundles long after a query limit has been reached. In the current
source limit would log the fact that it is happening and do nothing
but the culprit is actually in OpPivot which should stop sending
bundles once the query has been completed.

Matt

[Ian Barfield]

I understand the OpPivot relationship, but disk sort was specifically referenced in the change from exception to log line and at the end of the first email of this thread.

[Matt Abrams]

agree, sort checks status very often so it should break quickly once a
query is completed or canceled.

Do you have thoughts on the PR in general? safe to merge?

matt

[Ian Barfield]

Well, since we are adding the status observer to pretty much every constructor, we should be able to refer to it without adding it to the method signature. However, this PR also draws attention to the fact that QueryTableOp (the interface) extends QueryOp (the interface) and simply overrides a single method. I am a little surprised that is valid code. Aside from how confusing that is, this PR also breaks that relationship -- which is maybe a good thing? Partially as a result of the existing super-overloading, adding the observer as a method parameter probably also makes it less likely to miss something. It is still really hard to say whether it is safe without an exhaustive trace for any weird cases or overrides, but it is probably safe enough to be worth merging.

[Matt Abrams]

Agreed in general. Let's merge. There are better long term solutions
but I feel like this patch does more good than harm so its worth
having in. I'll merge. Our nightly tests run plenty of queries so it
has a good shot of identifying corner cases.

Matt