ISPN-5962 Implementation of RpcManagerImpl.invokeRemotely causes high CPU usage #3841
Conversation
| @@ -148,4 +150,17 @@ void invokeRemotelyInFuture(NotifyingNotifiableFuture<Map<Address, Response>> fu | |||
| */ | |||
| RpcOptions getDefaultRpcOptions(boolean sync, DeliverOrder deliverOrder); | |||
|
|
|||
| static Map<Address, Response> waitFor(CompletableFuture<Map<Address, Response>> future, RpcOptions options) | |||
| throws InterruptedException, ExecutionException { | |||
| long waitTime = options.timeout() * 2; | |||
There was a problem hiding this comment.
you have this exact same logic in https://github.com/infinispan/infinispan/pull/3841/files#diff-36bedadd971913e8bd8c08cc2e79e9a2R934 below. Maybe coalesce them ?
pruivo
force-pushed
the
ISPN-5962/compfuture_get
branch
4 times, most recently
from
03106ec
to
1ac00a1
Compare
|
@tristantarrant updated. I've created a |
| topologyAffectedCommand.setTopologyId(currentTopologyId); | ||
| } | ||
| } | ||
| setTopologyId(rpc); |
There was a problem hiding this comment.
I'd rather wait for a constant time, expressed in nanos so there's no conversion during the RPC (e.g. private static final long BIG_DELAY_NANOS = TimeUnit.DAYS.toNanos(1)).
pruivo
force-pushed
the
ISPN-5962/compfuture_get
branch
from
1ac00a1
to
38f7ffd
Compare
| private RpcUtil() { | ||
| } | ||
|
|
||
| public static <T> T waitFor(CompletableFuture<T> future) |
There was a problem hiding this comment.
One more (very late) suggestion: this should be useful for anything that needs to wait on a CompletableFuture, so I'd move it to CompletableFutures.
pruivo
force-pushed
the
ISPN-5962/compfuture_get
branch
from
38f7ffd
to
1205fa5
Compare
|
@danberindei updated! |
| @@ -502,7 +502,7 @@ public Address getAddress() { | |||
| CompletableFuture<Map<Address, Response>> future = invokeRemotelyAsync(recipients, rpcCommand, mode, | |||
| timeout, responseFilter, deliverOrder, anycast); | |||
| try { | |||
| return future.get(); | |||
| return CompletableFutures.await(future); | |||
There was a problem hiding this comment.
Could we add some comments here why we can afford to wait endlessly here? What is providing the guarantees that we are sure that we'll never wait endlessly?
There was a problem hiding this comment.
what do you mean by "wait endlessly"? all the rpc has a timeout.
There was a problem hiding this comment.
@pruivo Please add a comment saying the invokeRemotelyAsync is guaranteed to complete within timeout millis, so there's no need to use a timeout here.
… CPU usage * Cleanup some code in RpcManager
… CPU usage * Replace CompletableFuture.get() by CompletableFuture.get(long,TimeUnit).
pruivo
force-pushed
the
ISPN-5962/compfuture_get
branch
from
1205fa5
to
92849af
Compare
|
add the comment and rebased @galderz and @danberindei . |
|
Integrated, thanks Pedro! |
https://issues.jboss.org/browse/ISPN-5962
did some cleanups as well.