Skip to content

Add basic timeout functionality to QueryResource#1416

Closed
drcrallen wants to merge 2 commits intoapache:masterfrom
metamx:basicBrokerTimeout
Closed

Add basic timeout functionality to QueryResource#1416
drcrallen wants to merge 2 commits intoapache:masterfrom
metamx:basicBrokerTimeout

Conversation

@drcrallen
Copy link
Copy Markdown
Contributor

  • Add a waiting pool that can be used to timeout actions.
  • Does not stop queries which have partially returned results. Only stops queries which are completely blocked on the historical level.

@drcrallen
Copy link
Copy Markdown
Contributor Author

This is a partial solution for
#1415

A more complete solution will also interrupt the response correctly, but that is a bit more tricky since the response is already building an Ok, I'll probably need to check with @xvrl on how to handle that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason listeningDecorator(..) is needed and ExecutorService is not enough?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i am understanding it correct, its listeningexecutor so that the futures can be registered with QueryWatcher,
see QueryManager.registerQuery()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I needed to register

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, thanks

@drcrallen drcrallen force-pushed the basicBrokerTimeout branch from 1847e1e to 0db07c9 Compare June 3, 2015 15:40
@drcrallen drcrallen mentioned this pull request Jun 3, 2015
@xvrl
Copy link
Copy Markdown
Member

xvrl commented Jun 3, 2015

The main problem I see with adding a second thread pool where queries are waiting, is that we cannot guarantee that resulting yielders will always get closed, some of them may get discarded without being consumed, even if we cancel the future. If we don't close yielders, resources will not get released.

The other issue is that in the event of a large query backlog, we may potentially be creating hundreds of threads, since the pool in unbounded. Creating threads is expensive, so this will cause strain on the system, and compound the backlog even more.

A simpler solution might be to register the queries with the QueryManager before submitting them to the processing pool, and have the query processing ensure that already cancelled queries get interrupted before they even start doing any work as they are taken from the backlog.

@drcrallen
Copy link
Copy Markdown
Contributor Author

@xvrl : In the current way of doing things the thread pool will still explode, but it will be the http thread pool, So this pool shouldn't ever be larger than the http pool size.

The yielder not closing is definitely a problem with this approach. But looking at the different sequence.toYielder methods, some of them seem to properly closeout on error and others do not. This seems like a problem with sequences rather than this particular approach.

Having a check with the processing queue is reasonable, but still does not guarantee that the REST method will return in anything resembling the requested timeout.

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Jun 3, 2015

@drcrallen if there are toYielder methods with issues, we should fix those for sure.

Having the REST method timeout properly is always going to be tricky with the standard servlet model.

At the router level we handle this by setting the proper timeouts at the async servlet level. Maybe switching to async servlet here would be the right way to do things, since that comes with it's own separate non-blocking pool for I/O handling, and another pool for doing the work.

drcrallen added 2 commits June 3, 2015 14:38
* Add a waiting pool that can be used to timeout actions.
* Does not stop queries which have partially returned results. Only stops queries which are completely blocked on the historical level.
@drcrallen drcrallen force-pushed the basicBrokerTimeout branch from 0db07c9 to c683874 Compare June 3, 2015 21:40
@drcrallen
Copy link
Copy Markdown
Contributor Author

I split the test ( metamx@27e4e1f ) and proposed fix into different commits for easier changes down the line in this PR

@drcrallen
Copy link
Copy Markdown
Contributor Author

from AsyncQueryingServelet:

/**
 * This class does async query processing and should be merged with QueryResource at some point
 */

@drcrallen
Copy link
Copy Markdown
Contributor Author

Announcer test problems... restarting

@drcrallen drcrallen closed this Jun 3, 2015
@drcrallen drcrallen reopened this Jun 3, 2015
@drcrallen drcrallen closed this Sep 8, 2015
@drcrallen drcrallen reopened this Sep 8, 2015
@drcrallen drcrallen closed this Sep 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants