Combine spill strategies

[rschlussel]

Test plan - unit tests

== RELEASE NOTES ==

General Changes
* Remove spilling strategy ``PER_QUERY_MEMORY_LIMIT`` and instead add configuration property ``experimental.query-limit-spill-enabled`` and session property ``query_limit_spill_enabled``.  When this property is set to ``true``, and the spill strategy is not ``PER_TASK_MEMORY_THRESHOLD``, then we will spill whenever a query uses more than the per-node total memory limit in combined revocable and non-revocable memory, in addition to whenever the memory pool exceeds the spill threshold.  This fixes an issue where using the ``PER_QUERY_MEMORY_LIMIT`` spilling strategy could prevent the oom killer from running when the memory pool was full.  The issue is still present for the ``PER_TASK_MEMORY_THRESHOLD`` spilling strategy.

[pettyjamesm]

Looks good to me modulo a few minor comments added inline. I can't think of a better option to avoid concurrent memory pool and query revoking other than the single threaded executor approach taken here, which should be ok as long as expensive operations don't somehow migrate into the caller thread (potential risk: OperatorContext#memoryRevocationRequestListener and associated ListenableFuture<?> callbacks on Driver#driverBlockedFuture registered in Driver#initialize()).

[pettyjamesm]

Why not newSingleThreadExecutor(threadsNamed("memory-revocation"))? Seems like this is serving to enable the .getCorePoolCount() check in the primary constructor, but seems acceptable to omit the assertion if the executor creation is hard coded (maybe just move the comment to the creation site?).

The added benefit is that single threaded executors are wrapped in a FinalizableDelegatedExecutorService which will call shutdown if the executor is leaked without being shutdown first (a likely scenario in tests that might fail to call stop()).

[rschlussel]

Yeah, it was entirely to be able to get the corePoolCount. I'll change it.

[pettyjamesm]

Now that this instance is created in the constructor, it should be shutdown in the stop() method to avoid leaking it.

[rschlussel]

It wasn't created in the constructor for the tests, but I'll actually just change the tests to get the executor from here, and that will remove any need to do any checking about the pool size.

[rschlussel]

@pettyjamesm comments addressed and all tests passing

[pettyjamesm]

Clever, I didn't realize that invokeAll() blocked until completion like this.

[pettyjamesm]

Missing MemoryRevokingScheduler#stop() which would now leak the single threaded executor until GC finalization. Minor, but worth avoiding. Maybe a withMemoryRevokingScheduler(<params>, Consumer<MemoryRevokingScheduler>) helper could help with the boiler-plate of registerPoolListeners() and stop() but would be a problem with the way that InterruptedException is thrown from awaitAsynchronousCallbacksRun()...

Open to your thoughts about the best way to resolve the tension between test sanity and ensuring the revoking executor remains single threaded even with future refactoring changes.

[rschlussel]

ended up going with a simple try/finally pattern in the tests to start and stop the memory revoking scheduler