I think we should standardize on what passing

-1

means in terms of

queryFailTime

since I think this is another potential bug that should be addressed.

I'm generally skeptical to having anything <= 0 mean anything, especially since it risks correctness issues on code not checking for overflows

And I'm not really sure why we'd ever want to not bound the time a computation should take (meaning,

queryFailTime

should probably always be >>> 0) – perhaps if the calling code never sets the fail time parameter, and plans to enforce the timeout on caller-side vs callee?

I think any values > 0 should be considered valid queryFailTime values, and anything <= 0 should be treated as an immediate timeout exception.

Let me know what you folks think – I'm planning to merge the above PR as it's a adjacent bug but not critical to the above discussion.

(I'm of the opinion that every query should have a configured maximum timeout)

I just generally want to remove all possible cases where we can indefinitely block on some operation unknowingly (with the known cases being strictly enforced caller-side timeouts)

Let me take a closer look at these 2 classes and see if there's a valid reason for them to allow "indefinite" timeouts – seems like JsonParserIterator is the type of "dumb" class which shouldn't need to worry about a timeout at all (e.g. enforce it on the caller).

here's your original PR for that one for context

Might be worth considering deprecating this field and moving to a "millis left" relative notation – one that will be less prone to overflow and other things like clock skew between nodes.

@Clint Wylie if things look good on https://github.com/apache/druid/pull/19393 – could I get another stamp? I'll address this -1 thing in a follow-up.

i think it looks chill, approved