Test CDI and FT interceptors ordering changing FT interceptor priority #378

carlosdlr · 2018-12-20T17:53:53Z

FT interceptor priority issue 313

New tests added to validate the CDI interceptors against FT interceptor
when the FT interceptor priority is changed and the CDI interceptors
are injected to the context using xml or annotation base configuration.

Signed-off-by: carlos de la rosa kusanagi12002@gmail.com

FT interceptor priority issue 313 eclipse#313 New tests added to validate the CDI interceptors against FT interceptor when the FT interceptor priority is changed and the CDI interceptors are injected to the context using xml or annotation base configuration. Signed-off-by: carlos de la rosa <kusanagi12002@gmail.com>

eclipse-microprofile-bot · 2018-12-20T17:58:07Z

Can one of the admins verify this patch?

Azquelt · 2018-12-21T10:48:23Z

If I'm reading this right, in both cases (beans.xml and Priority annotation) you're changing the priority of the Fault Tolerance interceptor(s) in a way where you expect the same behaviour as when you didn't change the priority of it?

Could you change FaultToleranceInterceptorPriorityChangeAnnotationConfTest so that you set the FT interceptor priority in such a way that it changes the expected behaviour? (e.g. setting it to a lower value than the EarlyFtInterceptor or to a higher value than the LateFtInterceptor)

The other test is fine since when the interceptors are defined in the beans.xml, the priority of the FT interceptor shouldn't make any difference.

Azquelt · 2018-12-21T10:48:34Z

@genie-microprofile test this please

carlosdlr · 2018-12-21T12:45:01Z

Hello @Azquelt yes you are right I will make the change

Thanks

Emily-Jiang · 2019-01-08T17:30:31Z

@carlosdlr can you please make the change as indicated so that we can merge this PR? thanks

FT interceptor priority issue 313 eclipse#313 Fixes related to code review and use of the fault tolerance interceptor priority parameter. Signed-off-by: Carlos Andres De La Rosa <kusanagi12002@gmail.com>

Azquelt

Looks really good.

I have one request: can we change the interceptors so that the EarlyFtInterceptor is called earlier than LateFtInterceptor? Having the late interceptor called earlier than the early interceptor makes the test really confusing to read.

FT interceptor priority issue 313 eclipse#313 Fixes related to code review, order changed to make clear the test Signed-off-by: Carlos Andres De La Rosa <kusanagi12002@gmail.com>

Azquelt · 2019-01-10T09:08:56Z

.../interceptor/ftPriorityChange/FaultToleranceInterceptorPriorityChangeAnnotationConfTest.java

+        }
+
+        String [] expectedOrder = {"serviceRetryA","EarlyOrderFtInterceptor","LateOrderFtInterceptor",
+            "serviceRetryA","EarlyOrderFtInterceptor","LateOrderFtInterceptor"};


Unless I missed something, I was expecting this to be `{"EarlyOrderFtInterceptor","LateOrderFtInterceptor", "serviceRetryA", "EarlyOrderFtInterceptor","LateOrderFtInterceptor", "serviceRetryA"};

Hello @Azquelt,

This is not the case because the idea of this the is to validate that if the FT priority is changed to be called at first must work in the order that is expected in the test. if you check the priority values for this test are for the Earlier(Interceptor.Priority.PLATFORM_AFTER - 200) = 3800, Later(Interceptor.Priority.PLATFORM_AFTER - 100) = 3900 and for the FT the value in this test is 3700, so according to the interceptors documentation the order to be called is ascending so must be called FT, Early and Later interceptor.

Right, but even if the FT interceptor is called first, the method body is still called at the end.

The difference I expect to with the FT interceptor being first is that the interceptors are called on each retry, rather than only once at the start.

Ok so for no make sense this test because as a developer if I change the FT priority I expect some change in the order, I think is better wait after the implementation to add this test. What do you think?, because for me is not clear what is the purpose of this parameter (would you mind give me a brief explanation?). if you check the previous test related to this, the scenario that you mention "I expect to with the FT interceptor being first is that the interceptors are called on each retry, rather than only once at the start." is already tested and is working in the way that you mention without change the FT priority.

It looks like we have the following:

EarlyOrderFtInterceptor - priority 3800 LateOrderFtInterceptor - priority 3900 FT interceptor base - default priority 4010, but configured here to be 3700

If the FT interceptor is left at the default priority, I would expect the order to be "Early, Late, FT".

Therefore, in the order queue, I would expect to see {"EarlyOrderFtInterceptor","LateOrderFtInterceptor", "asyncGetString", "asyncGetString"}

If the FT interceptor is configured to priority 3700, I would then expect the order to be "FT, Early, Late".

Therefore, in the order queue, I would expect to see {"EarlyOrderFtInterceptor","LateOrderFtInterceptor", "asyncGetString", "EarlyOrderFtInterceptor","LateOrderFtInterceptor", "asyncGetString"}

The change is expected here because I now expect the interceptors to be called on each retry attempt.

Ok @Azquelt Thanks for your clarification

Azquelt · 2019-01-10T09:09:25Z

.../interceptor/ftPriorityChange/FaultToleranceInterceptorPriorityChangeAnnotationConfTest.java

+    public void testAsync() throws InterruptedException, ExecutionException {
+        Future<String> result = testInterceptor.asyncGetString();
+        assertEquals(result.get(), "OK");
+        String [] expectedOrder = {"asyncGetString","EarlyOrderFtInterceptor","LateOrderFtInterceptor"};


Similarly, should this not be {"EarlyOrderFtInterceptor","LateOrderFtInterceptor", "asyncGetString"} ?

Hello @Azquelt,

This is not the case because the idea of this the is to validate that if the FT priority is changed to be called at first must work in the order that is expected in the test. if you check the priority values for this test are for the Earlier(Interceptor.Priority.PLATFORM_AFTER - 200) = 3800, Later(Interceptor.Priority.PLATFORM_AFTER - 100) = 3900 and for the FT the value in this test is 3700, so according to the interceptors documentation the order to be called is ascending so must be called FT, Early and Later interceptor.

@Azquelt Changes done.

Thank you

FT interceptor priority issue 313 eclipse#313 Fixes related to code review, order changed to make clear the test Signed-off-by: Carlos Andres De La Rosa <kusanagi12002@gmail.com>

Azquelt · 2019-01-10T17:48:21Z

Thanks @carlosdlr :)

Emily-Jiang

LGTM! Thanks @carlosdlr !

Test CDI and FT interceptors ordering changing

e0e7023

FT interceptor priority issue 313 eclipse#313 Fixes related to code review and use of the fault tolerance interceptor priority parameter. Signed-off-by: Carlos Andres De La Rosa <kusanagi12002@gmail.com>

Azquelt reviewed Jan 9, 2019

View reviewed changes

Test CDI and FT interceptors ordering changing

78dc351

FT interceptor priority issue 313 eclipse#313 Fixes related to code review, order changed to make clear the test Signed-off-by: Carlos Andres De La Rosa <kusanagi12002@gmail.com>

Azquelt requested changes Jan 10, 2019

View reviewed changes

Test CDI and FT interceptors ordering changing

b07cb7a

FT interceptor priority issue 313 eclipse#313 Fixes related to code review, order changed to make clear the test Signed-off-by: Carlos Andres De La Rosa <kusanagi12002@gmail.com>

Azquelt approved these changes Jan 10, 2019

View reviewed changes

Emily-Jiang approved these changes Jan 11, 2019

View reviewed changes

Emily-Jiang merged commit 00ef8ca into eclipse:master Jan 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test CDI and FT interceptors ordering changing FT interceptor priority #378

Test CDI and FT interceptors ordering changing FT interceptor priority #378

carlosdlr commented Dec 20, 2018

eclipse-microprofile-bot commented Dec 20, 2018

Azquelt commented Dec 21, 2018

Azquelt commented Dec 21, 2018 •

edited

carlosdlr commented Dec 21, 2018 •

edited

Emily-Jiang commented Jan 8, 2019

Azquelt left a comment

Azquelt Jan 10, 2019

carlosdlr Jan 10, 2019 •

edited

Azquelt Jan 10, 2019

carlosdlr Jan 10, 2019 •

edited

Azquelt Jan 10, 2019

carlosdlr Jan 10, 2019

Azquelt Jan 10, 2019

carlosdlr Jan 10, 2019 •

edited

carlosdlr Jan 10, 2019

Azquelt commented Jan 10, 2019

Emily-Jiang left a comment

Navigation Menu

Test CDI and FT interceptors ordering changing FT interceptor priority #378

Test CDI and FT interceptors ordering changing FT interceptor priority #378

Conversation

carlosdlr commented Dec 20, 2018

eclipse-microprofile-bot commented Dec 20, 2018

Azquelt commented Dec 21, 2018

Azquelt commented Dec 21, 2018 • edited

carlosdlr commented Dec 21, 2018 • edited

Emily-Jiang commented Jan 8, 2019

Azquelt left a comment

Choose a reason for hiding this comment

Azquelt Jan 10, 2019

Choose a reason for hiding this comment

carlosdlr Jan 10, 2019 • edited

Choose a reason for hiding this comment

Azquelt Jan 10, 2019

Choose a reason for hiding this comment

carlosdlr Jan 10, 2019 • edited

Choose a reason for hiding this comment

Azquelt Jan 10, 2019

Choose a reason for hiding this comment

carlosdlr Jan 10, 2019

Choose a reason for hiding this comment

Azquelt Jan 10, 2019

Choose a reason for hiding this comment

carlosdlr Jan 10, 2019 • edited

Choose a reason for hiding this comment

carlosdlr Jan 10, 2019

Choose a reason for hiding this comment

Azquelt commented Jan 10, 2019

Emily-Jiang left a comment

Choose a reason for hiding this comment

Azquelt commented Dec 21, 2018 •

edited

carlosdlr commented Dec 21, 2018 •

edited

carlosdlr Jan 10, 2019 •

edited

carlosdlr Jan 10, 2019 •

edited

carlosdlr Jan 10, 2019 •

edited