Remove query.max-run-time.hard-limit

[martint]

It's not clear that this is the best approach to solve the problem described in the original PR. In particular:

• It adds yet another configuration option that users have to reason about
• It sets a bad precedent for every other option that may need a similar treatment
• It suffers from the problem that not even and administrator can override it in exceptional situations

In order to avoid having to back it out in a future version and break backward compatibility, we're removing it for now.

Other possible approaches include extending the access control APIs to allow checking the current and new values for the property to make a decision about whether the user is allowed to perform that action.

This reverts commit 5f7ae28.

Release notes

(x) This is not user-visible or docs only and no release notes are required.

[nevillelyh]

We need to remove https://github.com/trinodb/trino/blob/master/core/trino-main/src/test/java/io/trino/TestSystemSessionProperties.java as well @martint.

[findepi]

This reverts commit 5f7ae28.

It's not clear that this is the best approach to solve the problem described in the original PR.

The PR was reviewed for 11 days before being merged.
It was opened 27 days ago.

So it looks like 27 days is insufficient time to reach conclusion in something that looks rather straightforward from technical perspective. (At the same time non-trivial, mis-attributed, un-tested and under-reviewed code can get into the master branch within two hours of PR being open without raising anyone's concerns apparently.)

@martint how are we making decisions which PRs need to be scrutinized and stood corrected?

[colebow]

The PR was reviewed for 11 days before being merged. It was opened 27 days ago.

So it looks like 27 days is insufficient time to reach conclusion in something that looks rather straightforward from technical perspective. (At the same time non-trivial, mis-attributed, un-tested and under-reviewed code can get into the master branch within two hours of PR being open without raising anyone's concerns apparently.)

@martint how are we making decisions which PRs need to be scrutinized and stood corrected?

I feel like you're bringing up a different discussion which should be had on that PR (or Trino Slack).

#15115 is technically straightforward and I don't think there's any argument about that; it's relatively evident that it's receiving scrutiny for design/architecture reasons. It doesn't seem to me that there have been any process concerns or failings beyond it taking a while to resolve. The focus here should be on coming to a conclusion as to whether or not we want to keep it.

Combining concerns about #15365 with this is going to muddy discussion and make things more adversarial than they need to be. It's worth having a conversation about, but I feel like each decision should be independent and not some strange quid pro quo arrangement. Ideally, we resolve both issues quickly, independently of each other and on their own merits, so we can unblock the release.

[hashhar]

For query run time hard limits another option is to overload the meaning of resource-overcommit. We also don't allow raising memory limits beyond the config but only allow lowering them.

So we can do:

• By default don't allow raising query runtime via session higher than what is configured
• If resource-overcommit is enabled then allow session value to go higher than what is configured

It's breaking change (since default behaviour changes) but it's easy to reason about. Any other toggles we want to constraint to not go above system config can be restricted in this way and unlocked via resource-overcommit.

[findepi]

For query run time hard limits another option is to overload the meaning of resource-overcommit.

You mean the resource_overcommit session property? (that currently has no config?)
It would work for the use-case of node draining, if we ALSO add a change to node graceful shutdown to wait for query completion, except when queries are using resource overcommit and already running longer then otherwise they would.

While it will also satisfy the requirements, I am worried about complexity it would introduce.

The other problem is bundling functionalities.
resource_overcommit for years allow going over memory resources, at the risk of getting killed.
Wrt time limit, Trino default behavior has been to let users bump max running time.
Now, if users will want to restore the previous default behavior, they will need to unlock resource overcommit, risking their queries getting killed. Might be not awesome.