Fail-fast if max-idle-seconds is set below 2 seconds

[galibey]

Fixes: #14697

[mustafaiman]

minor comment about doc. Otherwise looks good to me 👍

[mustafaiman]

I am not arguing my english is perfect here but I think the message must be "greater than or equal to"

[galibey]

You're right. Fixed. Thanks.

[mustafaiman]

the same issue with "greater than or equal to"

[galibey]

Fixed

[mmedenjak]

One concern - what if the user is already using IMap with expiration and is setting the TTL to 1 second? We will start throwing IllegalArgumentException with this PR. @tkountis WDYT? Is this a plausible scenario? And better question - if the user is using a TTL of 1 second, is it a valid setting at all?

[mustafaiman]

@mmedenjak
I wrote this test into BasicMapTests

    @Test
    public void testOneSecondTtl() {
        final IMap<String, String> map = getInstance().getMap(randomString());
        long start, end;
        start = System.currentTimeMillis();
        for (int i = 0; i < 300000; i++) {
            map.put("key", "value", 2000, MILLISECONDS);
            end = System.currentTimeMillis();
            assertEquals("Iteration " + i + ". Ms until now: " + (end - start), "value", map.get("key"));
        }
    }

It takes about 20 seconds on my machine and test passes fine. When I change TTL value to 1999 milliseconds, the test fails within the first second. I guess having a 1 second TTL does not make sense either. Because we round down creation time. If the entry is created at x4998ms time offset, we record it as x4000 by rounding down. When we check elapsedMillis during map.get, most of the time elapsed time is larger than 1000ms.

[galibey]

@mmedenjak
Here we added a validation only for MaxIdle. For TTL it's still possible to define 1s.
For TTL even with 1s it will work more or less as expected, but with MaxIdle=1s it will never work, the entry will always be evicted regardless of user's touches.

[mmedenjak]

Ok, ok, I'm convinced :) Maybe we can add this as a note in the changelog cc @Serdaro

[ahmetmircik]

4 concerns with this change:

• fail-fast should only work for cluster-versions >= 3.12, now this check is not rolling upgrade friendly.
• tend to see 1sec as a legitimate value to set, even this is a rare case. Mostly seems, we are leaking an implementation detail to public api.
• since this is not a backward compatible change lots of existing code can fail in a minor release.
• in the future, if we unify imap and icache code, this change can become a headache point since icache supports 1secs but imap does not.

[mmedenjak]

@ahmetmircik It's a tricky question. I guess this was introduced because we shaved off some data to make HD records more compact, which is great. Now, we have this quirky behaviour.
Reverting the HD record format isn't a good option because everyone loses because of a few which actually care about higher precision.
Having a test like the one Mustafa posted fail is very counterintuitive. The entry is mutated almost constantly but it's expired nevertheless.
But I must admit this feels a bit like we're leaking implementation details and it may be disruptive to some users. It's a detail that we currently don't support such precision and it may be fixed in some other way. I'm going to go with some middle ground:

• the exceptions should be removed
• can you look into whether there's a way that we can signal that the entry was, in fact, mutated during the first second and avoid the issue?
• if not, can you leave some documentation in javadoc and reference manual that setting max-idle-seconds to 1 second can lead to unexpected behaviour.

[galibey]

@mmedenjak

• can you look into whether there's a way that we can signal that the entry was, in fact, mutated during the first second and avoid the issue?

Without increasing precision it won't help much, e.g.,
first access occurred at 0.001s, we count as 1s, and then, second access was at 1.999s, we count as 2s. So, the entry will still live although almost 2s passed.

I agree that we should remove a throwing of the exception and add to javadoc that our current precision is 1s and all below 1s can lead to unexpected behaviour.