Remove query.low-memory-killer.delay

[sopel39]

query.low-memory-killer.delay is not needed:

• It does not prevent cascades of query kills. That is achieved by isLastKillTargetGone check in ClusterMemoryManager

• OOM blocked worker cannot be unblocked by other means other than killing the query. Revocable memory (and spill to disk) also won't cause node to be considered out-of-memory. Hence low-memory-killer does not interfere with spill-to-disk.

  Having query.low-memory-killer.delay causes reduced concurrency
  on a cluster with higher concurrency and under low-memory situations.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# General
* Remove `query.low-memory-killer.delay` config property which should improve query concurrency
  in low-memory situations. ({issue}`issuenumber`)

[electrum]

The delay is there so that memory can be freed on worker nodes. Is there another way this is achieved?

[sopel39]

The delay is there so that memory can be freed on worker nodes. Is there another way this is achieved?

Memory won't release on it's own on the workers.

In the code after changes there is:

        if (!lowMemoryKillers.isEmpty() && outOfMemory && !queryKilled) {
            if (isLastKillTargetGone()) {
                callOomKiller(runningQueries);
            }
            else {
                log.debug("Last killed target is still not gone: %s", lastKillTarget);
            }
        }

so we still wait until the previous target query goes away. The previous code was:

        if (!lowMemoryKillers.isEmpty() &&
                outOfMemory &&
                !queryKilled &&
                nanosSince(lastTimeNotOutOfMemory).compareTo(killOnOutOfMemoryDelay) > 0) {
            if (isLastKillTargetGone()) {
                callOomKiller(runningQueries);
            }
            else {
                log.debug("Last killed target is still not gone: %s", lastKillTarget);
            }
        }

so it would loop over queries (when isLastKillTargetGone is true and node is still OOM) without any extra delay. Hence, the delay was redundant anyway.

[wendigo]

Soft approval from me (code looks ok, logic seems sound but I'm not an expert in that area). @electrum ptal

[losipiuk]

Seems fine to me. Looks like the delay may only be used so we allow node-local memory limits to trigger before cluster level memory limits trigger. But it does not seem important.

[sopel39]

Seems fine to me. Looks like the delay may only be used so we allow node-local memory limits to trigger before cluster level memory limits trigger. But it does not seem important.

max mem per node query limits are enforced io.trino.memory.QueryContext#updateUserMemory even before MemoryPool gets updated on worker