added diagnostics support for @Retry annotation member values

[AlexXuChen]

Added diagnostics support for @Retry annotation member values, for non-negative values and delay exceeding maxDuration

References #77

Signed-off-by: Alexander Chen alchen@redhat.com

[rgrunber]

It works, but there's a small problem I noticed. It only makes sense to compare the delay and maxDuration directly when their time units (delayUnit & durationUnit) match. If they don't, you may need to do some additional math (or api call) to be sure.

For example If delay = 5, maxDuration = 3, delayUnit = ChronoUnit.SECONDS, durationUnit = ChronoUnit.MILLENNIA, validation would currently fail, but we should be fine.

[AlexXuChen]

According to the docstring for jitter(https://github.com/eclipse/microprofile-fault-tolerance/blob/800363fe002701bb2e31ba413f21e5a36af56b41/api/src/main/java/org/eclipse/microprofile/faulttolerance/Retry.java#L120), I believe that the jitter can randomly add to/subtract from the delay. That means if the delay + jitter exceeds maxDuration, then it should be invalid. However, there doesn't seem to be a test case for this on the mp fault-tolerance side.

Should this case be considered in validation @rgrunber?

[rgrunber]

I think at this point I'd be inclined to just validate if the time units are the same. Seems overkill to check jitter as well.

[rgrunber]

@angelozerr , looks like RangeExpression.parse(String) can't deal with the - from -1 . We need such values as sometimes it can indicate "infinite wait" for things like a delay.

org.eclipse.lsp4mp.jdt.internal.core.java.validators.annotations.RangeExpressionException: Unexpected character - at 0 for expression -1

[rgrunber]

I think we just need to wait on a fix for lsp4mp to handle negative values in the range.

[rgrunber]

Can you try adding :

diff --git a/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/src/main/java/org/eclipse/lsp4mp/jdt/internal/core/java/validators/annotations/RangeExpression.java b/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/src/main/java/org/eclipse/lsp4mp/jdt/internal/core/java/validators/annotations/RangeExpression.java
index 9b70b92..82cd29a 100644
--- a/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/src/main/java/org/eclipse/lsp4mp/jdt/internal/core/java/validators/annotations/RangeExpression.java
+++ b/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/src/main/java/org/eclipse/lsp4mp/jdt/internal/core/java/validators/annotations/RangeExpression.java
@@ -152,7 +152,7 @@ public class RangeExpression {
                                valueAsString.append(c);
                                break;
                        default:
-                               if (Character.isDigit(c)) {
+                               if (Character.isDigit(c) || '-' == c) {^M
                                        valueAsString.append(c);
                                } else {
                                        unexpectedToken(c, i, expression);

and see if that solves the issue ?

Note : Proper approach is to give it its own case where valueAsString.length() should be 0 or else it's an unexpected token.

[AlexXuChen]

and see if that solves the issue ?

This is a solution, but I also added the case '-': https://github.com/eclipse/lsp4mp/pull/191/files#diff-3e4d6d0d761c9ffc6f9f9d20379ec87c91773ab5778f245110280ad427691a5fR154 which seems to handle the -

[angelozerr]

LGTM

[angelozerr]

Great job @AlexXuChen !

[rgrunber]

Try the following :

@Retry(
        delay = 8,
        maxDuration = 2,
        delayUnit = java.time.temporal.ChronoUnit.HOURS,
        durationUnit = ChronoUnit.HOURS
    )

Basically, you need to deal with fully qualified names.

A fully qualified name is made of 2 components : The qualifier (eg. java.time.temporal.ChronoUnit, or just ChronoUnit) and the name (eg. HOURS).

[rgrunber]

Ok, jitter it is. It isn't just added to the delay, but also subtracted. It adds some random delay to the minium value. Have a look at https://github.com/eclipse/microprofile-fault-tolerance/blob/800363fe002701bb2e31ba413f21e5a36af56b41/api/src/main/java/org/eclipse/microprofile/faulttolerance/Retry.java#L113 .

The reason I hesitated is because if you take a delay of 3, with a jitter of 2, and maxDuration of 4, you'll have delays between [1,5] . Some of the delays will fall below 4, but others above. Should we all this an error ? Maybe a warning ?

[AlexXuChen]

Currently the error message reports "The delay member value must be less than the maxDuration member value.", but to illustrate this, I can mention "the sum of delay and jitter...".

According to https://github.com/eclipse/microprofile-fault-tolerance/blob/800363fe002701bb2e31ba413f21e5a36af56b41/api/src/main/java/org/eclipse/microprofile/faulttolerance/Retry.java#L113, the negative effective delays have a lower bound of 0, which means the maximum effective delay should be the only one we need to validate.

[rgrunber]

You can also declare : import static java.time.temporal.ChronoUnit.HOURS; and then use HOURS directly, in which case it won't be a QualifiedName, but just a SimpleName. This and the above will break your validation.