Fix TimeValue comparisons for -1 duration

[JeremyDahlgren]

-1 duration for TimeValue has special meaning and is the only negative value allowed in the constructor. When a TimeValue string is parsed, "-1" is converted into TimeValue.MINUS_ONE, which uses TimeUnit.MILLISECONDS. Round trip equality of TimeValue.timeValueSeconds(-1) to string form and back will fail since the time units will be different in compareTo(). This change adds a special check in compareTo() to return zero if both durations are -1. Unit test cases are also added that fail without this change.
I ran into this in a unit test when checking a time setting default value, it boiled down to a check similar to the one below which seemed reasonable, but was failing:

        final var DEFAULT_TIME_VALUE = TimeValue.timeValueSeconds(-1);
        final var timeSetting = timeSetting("key", DEFAULT_TIME_VALUE);
        assertThat(TimeValue.timeValueSeconds(-1), equalTo(timeSetting.getDefault(Settings.EMPTY)));

[elasticsearchmachine]

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

[rjernst]

Shouldn't we also handle one being -1 and the other not?

[DaveCTurner]

I think this is ok: if one is negative and the other is non-negative then the negative one will correctly compare as less than the non-negative one already.

[DaveCTurner]

The -1-with-units case for time values is pretty weird and IMO there's a strong argument that permitting them in the first place was a bug. The intention was that the string -1 is a reasonable representation of a missing time value, e.g. an infinite timeout, but it is an implementation detail that this means the same as -1ms and there's no good reason for us to be able to distinguish "a missing number of milliseconds" from "a missing number of seconds".

Given that these cases exist today I'd be inclined to treat them all differently: -1s is a smaller (more negative) time value than -1ms. That would mean that the bug is that getStringRep doesn't faithfully represent these things, and I'd be supportive of fixing that.

Then, separately, I'd like us to work towards dropping support for -1-with-units time values altogether. They aren't documented anywhere AFAIK and I think it's a bug that they're even permitted, so deprecating them now and removing them in the future should have minimal impact.

In the meantime I'd suggest we could add a little bit of lenience here and change the parser to map strings -1s, -1m, -1d etc onto -1, likewise adjusting StreamInput#readTimeValue to overwrite timeUnit as MILLISECONDS if duration is negative, and rejecting all other attempts to create a negative timevalue in code. That way we'd never see any of these odd negative TimeValue instances apart from TimeValue.MINUS_ONE.

[DaveCTurner]

LGTM assuming CI is happy too

[DaveCTurner]

The failing test is bogus:

elasticsearch/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/ml/forecast.yml

Lines 59 to 64 in 1f5e04b

	"Test forecast given expires_in is negative":
	- do:
	catch: /\[expires_in\] must be non-negative[:] \[-1\]/
	ml.forecast:
	job_id: "forecast-job"
	expires_in: "-1s"

AFAICT there's nothing in any docs or anything to say that -1s is a valid value for this expires_in field. I think we should fix the test to just use -1, in a separate PR backported to all relevant branches, which should then mean that this REST compatibility test will pass.