Also addresses #654 and #655, right?

Yes, although it doesn't have anything related to multi-host support at the moment. If you want to kill a query, you have to run it against that specific node.

@jsternberg Why is there an InterruptIterator? Shouldn't we be able to call Iterator.Close() on the top level iterators and have it trickle down?

I'll try it out. I can't remember if I tried that or not.

The reason it was done this way was so that a channel could be closed from one of a few different places and it would automatically close anybody who was subscribed to that channel. It means I could just pass the channel into each iterator. The interrupt has to be done at multiple points in the engine so there need to be several places where the interrupt is checked.

I couldn't really find a way to interrupt in the middle of a call to Next() without a decent amount of difficulty. For example this query:

SELECT count(value) FROM cpu

Will only return a single point from a call to Next(). If it's closed at the top level in the emitter, it won't interrupt the currently running Next() call. Call like SELECT value FROM cpu are fine and would work correctly, but not the emitter.

That's the rationale for the interrupt iterator.

@jsternberg I can understand needing to use a lock or a channel at some point lower down in the iterator chain to stop the lower Next() but that can still be implemented using Iterator.Close(). Adding a separate InterruptCh makes two different ways to close an iterator.

@benbjohnson I don't think that interpretation is correct. The channel is used as a side-channel to interrupt the iterator. The Close() call releases used resources and provides the cleanup for the iterator. The channel does not clean up any resources and using Close() is required even if the iterator is interrupted. Without doing it in this way, I would have to handle the mutex or channel for interrupting the iterator in every iterator type rather than it just being part of IteratorOptions.

I think of it as more similar to how Unix signals are implemented and the interrupt iterator is the part that handles the signal interrupt. I've been trying to do it with close and it involves spawning a new goroutine at the top level and ensuring that all Close() calls are thread-safe. Currently, Close() calls happen from the same thread as Next() so they don't have to be thread-safe.

The Next() doesn't have to be thread-safe on every iterator. Most iterators simply bubble the Close() up to the input. In the case of an iterator that streams remotely it will close the connection and stop points from being read by Next().

You can still implement something like the interrupt iterator that handles the Close() in a thread-safe manner while not touching the other iterators.

I don't think it's wise to conflate the interrupt operation with a close operation. A close operation is designed to clean up resources while an interrupt is usually an asynchronous interruption that is easily facilitated through channels.

I used the method for how interrupts are implemented with Java threads here instead of trying to make interrupts use the close method: https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html

I also think it follows with the same idea of how Unix signals and process management are done on Linux. An interrupt is a request that the iterator (or process in Linux) be closed, not a demand.

I also think there's another problem with using Close() as the method of implementing interrupts. When an interrupt happens, an iterator can continue until it hits one of its interrupt points. If we implement that with Close(), then should Close() return immediately or wait until the iterator actually closes? That then requires two channels so that Close() can signal to Next() it's supposed to stop and then for Next() to signal to Close() that it actually stopped (which may or may not happen as an iterator can be interrupted after all points have already been read, so Next() would never be able to signal to Close()). If it closes immediately, then Close() can't clean up any used resources from the parent iterator until the parent iterator acknowledges the signal. So for an asynchronous close, you have the following operations:

Close() -> signals the iterator should be closed
Next() -> returns a point
Next() -> received signal, sends nil as point
Close() -> cleans up resources

The problem is that the two closes don't know which is which. They're different operations. I also think it works better with Go idioms. Channels are meant for different goroutines to communicate with each other. We would be sharing memory between multiple goroutines by having Close() be called from multiple goroutines. Sharing memory by communication is preferred to sharing memory directly: https://blog.golang.org/share-memory-by-communicating

I think you're overcomplicating the Close() idea. It doesn't need to be synchronous. I agree that Java & Unix use a different approach but io.Closer is the most common idiom within Go for shutting something down. How it's handled within the Close() method is up to the implementation.

The Close() doesn't need to be synchronous just as calling net.Conn.Close() isn't synchronous. It signals to the reader of the connection that it's closed and the reader can handle the returned error. For the iterator, any Next() called after Close() will return a nil point. We need to change Next() to return an error too but that's a bigger discussion.

Adding an interrupt channel means that we have to pass it down in the IteratorOptions which doesn't make sense and then the options won't serialize the channel across to distributed nodes so then we have different behavior on local nodes vs remote nodes. The Close() method already handles this by notifying the remote iterator that the connection is closed.

I've renamed the DetachQuery stuff to KillQuery and DetachCh to InterruptCh. I also implemented the first query limit with a query timeout. It defaults to 5 minutes, but maybe it should be 1 minute instead. This adds one of the limits mentioned in #6024.

I know MySQL has KILL QUERY, but does it make more sense to use a DROP QUERY syntax to be consistent with the rest of InfluxQL?

KILL is used for things that are analogous to proceeses (eg queries). We don't have an explicit CREATE QUERY so I don't think DROP QUERY necessarily makes sense.

Fixes #675

I'm removing the timeout related stuff from this PR so it can be part of a separate PR.