Fixes in RPC handling #4486

rvansa · 2016-08-04T12:12:46Z

https://issues.jboss.org/browse/ISPN-6925
https://issues.jboss.org/browse/ISPN-6795

The third commit does not have a JIRA as it is a fix of consequences of ISPN-6925.

rvansa · 2016-08-04T15:04:28Z

core/src/main/java/org/infinispan/interceptors/distribution/BaseDistributionInterceptor.java

@@ -243,7 +246,17 @@ public void configure() {
         if (rvrl != null) {
            rvrl.remoteValueNotFound(key);
         }
-         return null;
+         if (!hasSuccesfulResponse) {
+            RpcException rpcException = new RpcException("No successful response: " + responses);


Actually it's a bit weird that remote node will return SuccessfulResponse{null} when it does not find the entry locally, I think that UnsuccessfulResponse would make more sense...

Indeed. Actually we used to send an UnsureResponse back in such cases (with a check in DistributionResponseGenerator). But because ClusteredGetCommand doesn't have a topology, the check we did there was almost guaranteed to be false, so I finally removed it with ISPN-5463.

Besides, it's weird to have a locality check in the response generator, I'd rather remove the ResponseGenerator and create the Response in the command itself. (Or maybe in the interceptors...)

galderz · 2016-08-23T08:26:30Z

Needs rebasing

danberindei · 2016-08-23T18:05:27Z

core/src/main/java/org/infinispan/remoting/responses/ClusteredGetResponseValidityFilter.java

@@ -33,7 +33,7 @@ public ClusteredGetResponseValidityFilter(Collection<Address> targets, Address s
   public boolean isAcceptable(Response response, Address address) {
      if (targets.contains(address)) {
         missingResponses--;
-         if (response instanceof SuccessfulResponse) {
+         if (response instanceof SuccessfulResponse || response instanceof ExceptionResponse) {
            validResponses++;


ExceptionResponse is not a ValidResponse, so the validResponses field name is no longer accurate.

danberindei · 2016-08-24T07:58:57Z

@rvansa Are you sure the 3rd commit is needed because of the ISPN-6925 fix and not because of the ISPN-6795 fix? To me, it seems like the ISPN-6925 fix should be sufficient.

And this PR could really use a test...

rvansa · 2016-09-02T08:03:26Z

@danberindei Hi, I've added the tests.

danberindei · 2016-09-02T14:53:19Z

core/src/test/java/org/infinispan/distribution/RemoteGetFailureTest.java

+   @Override
+   protected void createCacheManagers() throws Throwable {
+      ConfigurationBuilder builder = getDefaultClusteredCacheConfig(CacheMode.DIST_SYNC);
+      // cache stop takes quite long when the view splits


Could it be waiting for remote txs to finish, because of cacheStopTimeout?

It's non-tx cache. This really waits for ST.

Ok, I guess that's another bug then :)

rvansa · 2016-09-13T12:28:02Z

@danberindei Rebased & addressed.

slaskawi · 2016-09-14T12:07:18Z

LGTM, @danberindei May I integrate it?

rvansa · 2016-09-14T14:56:08Z

core/src/main/java/org/infinispan/remoting/responses/ClusteredGetResponseValidityFilter.java

@@ -33,8 +33,8 @@ public ClusteredGetResponseValidityFilter(Collection<Address> targets, Address s
   public boolean isAcceptable(Response response, Address address) {
      if (targets.contains(address)) {
         missingResponses--;
-         if (response instanceof SuccessfulResponse) {
-            validResponses++;
+         if (response instanceof SuccessfulResponse || response instanceof ExceptionResponse) {


@pruivo @tristantarrant @galderz @wburns Hi, so we have a stalemate with @danberindei we'd like to decide:

When a remote get is executed (to say, 2 owners), and first owner responds with an exception, there are two things we can do:

ignore the exception (swallow it) and wait for the second response (possibly without exception) - that's what master does

report the exception immediately (suggested change)

An exception can be thrown from two reasons:
a) bug in Infinispan code
b) AvailabilityException or similar

Note: AE can be thrown on one owner and not on another under certain timing; it should not happen in steady state

Pros:

fail fast, both during development (for a)-style exceptions), and for b) as the user code is likely to hit AE more often (so the code will deal with it properly). Swallowing exceptions hides problems.

when all owners would throw an exception, we won't get just RpcException without any other info (until this PR, it would end up with timeout, other fixes in the PR at least throw RpcException when there's no acceptable response)

don't do another RPC when the first one already failed - just hoping for the best when we're likely to get the same response

Cons:

the value can be available, and could be of use to the user (rather than the exception)

it is a change of behaviour (in a situation when the exception can happen with different timing, though)

Since we are talking about Get commands, I vote for option 1 (ignoring exception and waiting for second response).

What about OutdatedTopologyExceptions ? (still not a steady state)

@tristantarrant OTEs are not thrown with ClusteredGetCommand (yet), because it does not have topologyId. I'd like to change that in another PR (not finished yet), so you can consider it as well - it's quite similar to the AE.

Other opinions?

Okay, my vote goes to 1. I guess we'd want the opinions of @wburns and @pruivo

Since others' opinion seems to be null, I'll take the result as 1).

@tristantarrant Turns out our current behaviour is actually 2), with the caveat that we delay throwing the exception until we get a response from all the owners. We started behaving like 1) after the staggered gets work, but because of a bug that happened only when staggered gets were actually disabled.

Turns out supporting 1) and reporting meaningful exceptions is also pretty awkward with the ResponseFilter API, so I agreed with @rvansa to converge to 2) everywhere, at least until we implement our own response handling on top of JChannel.

rvansa · 2016-09-15T13:36:28Z

Added one more commit, fixing an issue when a request times out but TimeoutException is not thrown.
The test included in the PR would fail (without the fix) with RpcException.

rvansa · 2016-09-15T16:12:38Z

Rebased to master.

* also fixes issue when a response marked as suspected was copied as received with null value. * when a timeout is reached, TimeoutException is thrown (possibly with suppressed exceptions)

* updated check in PartitionHandlingInterceptor to test exceptions instead of null return value.

* provided as separate commit to allow running the test easily on master

danberindei · 2016-09-20T13:19:34Z

Integrated, thanks Radim!

rvansa added the Ready for Review label Aug 4, 2016

rvansa assigned danberindei Aug 4, 2016

rvansa mentioned this pull request Aug 4, 2016

ISPN-6645 Scattered cache #4458

Closed

rvansa force-pushed the t_rpc branch from f1d38c5 to c46e953 Compare August 4, 2016 14:50

rvansa reviewed Aug 4, 2016
View reviewed changes

rvansa mentioned this pull request Aug 9, 2016

Enhancements for Functional Map API #4493

Closed

slaskawi added Needs Rebase and removed Ready for Review labels Aug 22, 2016

danberindei reviewed Aug 23, 2016
View reviewed changes

danberindei added the Changes Suggested label Aug 24, 2016

rvansa force-pushed the t_rpc branch from c46e953 to 4fd58ed Compare September 2, 2016 07:59

rvansa added Ready for Review and removed Changes Suggested Needs Rebase labels Sep 2, 2016

rvansa force-pushed the t_rpc branch from 4fd58ed to 6ed2afb Compare September 2, 2016 10:24

danberindei reviewed Sep 2, 2016
View reviewed changes

danberindei added Changes Suggested and removed Ready for Review labels Sep 2, 2016

rvansa force-pushed the t_rpc branch from 6ed2afb to a3e57b6 Compare September 13, 2016 12:27

rvansa added Ready for Review and removed Changes Suggested labels Sep 13, 2016

rvansa force-pushed the t_rpc branch from a3e57b6 to 5080ede Compare September 14, 2016 10:31

rvansa reviewed Sep 14, 2016
View reviewed changes

slaskawi added the Question label Sep 15, 2016

rvansa force-pushed the t_rpc branch 2 times, most recently from 7116432 to 170587f Compare September 15, 2016 16:11

rvansa force-pushed the t_rpc branch from 170587f to b00aa2c Compare September 15, 2016 16:14

rvansa added 4 commits September 19, 2016 16:33

ISPN-6795 ClusteredGetResponseValidityFilter swallows exceptions

e9f6706

ISPN-6925 Race condition in staggered gets

d4c99d9

* also fixes issue when a response marked as suspected was copied as received with null value. * when a timeout is reached, TimeoutException is thrown (possibly with suppressed exceptions)

Throw exception when missing successful response

3cb6007

* updated check in PartitionHandlingInterceptor to test exceptions instead of null return value.

ISPN-6925 Test case

aa31aae

* provided as separate commit to allow running the test easily on master

rvansa force-pushed the t_rpc branch from b00aa2c to aa31aae Compare September 19, 2016 14:44

danberindei closed this Sep 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes in RPC handling #4486

Fixes in RPC handling #4486

rvansa commented Aug 4, 2016 •

edited by danberindei

rvansa Aug 4, 2016 •

edited

danberindei Aug 11, 2016

galderz commented Aug 23, 2016

danberindei Aug 23, 2016

danberindei commented Aug 24, 2016

rvansa commented Sep 2, 2016

danberindei Sep 2, 2016 •

edited

rvansa Sep 12, 2016

danberindei Sep 14, 2016

rvansa commented Sep 13, 2016

slaskawi commented Sep 14, 2016

rvansa Sep 14, 2016

slaskawi Sep 15, 2016

tristantarrant Sep 15, 2016

rvansa Sep 15, 2016

rvansa Sep 16, 2016

tristantarrant Sep 16, 2016

rvansa Sep 19, 2016

danberindei Sep 20, 2016

rvansa commented Sep 15, 2016

rvansa commented Sep 15, 2016

danberindei commented Sep 20, 2016

Fixes in RPC handling #4486

Fixes in RPC handling #4486

Conversation

rvansa commented Aug 4, 2016 • edited by danberindei

rvansa Aug 4, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galderz commented Aug 23, 2016

Choose a reason for hiding this comment

danberindei commented Aug 24, 2016

rvansa commented Sep 2, 2016

danberindei Sep 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvansa commented Sep 13, 2016

slaskawi commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvansa commented Sep 15, 2016

rvansa commented Sep 15, 2016

danberindei commented Sep 20, 2016

rvansa commented Aug 4, 2016 •

edited by danberindei

rvansa Aug 4, 2016 •

edited

danberindei Sep 2, 2016 •

edited