Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduction of async atomic long API #7960

Merged
merged 1 commit into from Apr 19, 2016

Conversation

Projects
None yet
4 participants
@vbekiaris
Copy link
Contributor

commented Apr 15, 2016

This PR fixes #7957 by providing async variants of existing methods in IAtomicLong. All async methods return ICompletableFuture, enabling reactive style invocation of execution callbacks by means of andThen(...). @Beta annotation was removed from ICompletableFuture which exists since version 3.2. AsyncAtomicLong is marked as deprecated and will be removed in a future version.

@jerrinot jerrinot added this to the 3.7 milestone Apr 15, 2016

@vbekiaris vbekiaris force-pushed the vbekiaris:async-methods-atomiclong-7957 branch from 5ba3d64 to af34ec7 Apr 15, 2016

@@ -77,6 +84,25 @@ public void test() throws Exception {

}

@Test

This comment has been minimized.

Copy link
@pveentjer

pveentjer Apr 15, 2016

Member

We definitely need to fix the client testing so that these tests don't need to be duplicated from the server.

@@ -66,6 +67,11 @@ public long addAndGet(long delta) {
}

@Override

This comment has been minimized.

Copy link
@pveentjer

pveentjer Apr 15, 2016

Member

Could you change the implementation so that the deprecated 'asyncXXX' are forwarding to the new 'XXXAsync'? This way it is a lot easier to get rid of the deprecated methods instead of having more reliance on them.

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Apr 15, 2016

Author Contributor

Yes, sure.

@vbekiaris vbekiaris force-pushed the vbekiaris:async-methods-atomiclong-7957 branch from af34ec7 to 62fb25d Apr 15, 2016

@Override
public ICompletableFuture<Long> getAsync() {
ClientMessage request = AtomicLongGetCodec.encodeRequest(name);
return invokeOnPartitionAsync(request, DECREMENT_AND_GET_DECODER);

This comment has been minimized.

Copy link
@sancar

sancar Apr 15, 2016

Member

Should have been GET_ASYNC_DECODER ?

@sancar

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

👍

@vbekiaris vbekiaris force-pushed the vbekiaris:async-methods-atomiclong-7957 branch from 62fb25d to 6ec8f0d Apr 15, 2016

@vbekiaris vbekiaris force-pushed the vbekiaris:async-methods-atomiclong-7957 branch from 6ec8f0d to dee97fe Apr 15, 2016

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2016

Thanks for your comments, above issues are now addressed.

@pveentjer

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

On the serverside there is one 'true' method, the async version. And when the sync version is called, it forward to the async and then calls join. I love this because only at the last moment it becomes visible that you are going to do a blocking call; apart from that there is zero difference.

On the client side the logic for sync is copied. Would it not make sense to do on the client the same as on the server.

before

@Override
public <R> ICompletableFuture<R> applyAsync(IFunction<Long, R> function) {
        isNotNull(function, "function");
        ClientMessage request = AtomicLongApplyCodec.encodeRequest(name, toData(function));
        return invokeOnPartitionAsync(request, APPLY_DECODER);
}

@Override
public <R> R apply(IFunction<Long, R> function) {
    isNotNull(function, "function");
        ClientMessage request = AtomicLongApplyCodec.encodeRequest(name, toData(function));
        ClientMessage response = invokeOnPartition(request);
        AtomicLongApplyCodec.ResponseParameters resultParameters = AtomicLongApplyCodec.decodeResponse(response);
        return toObject(resultParameters.response);
}

after

@Override
public <R> ICompletableFuture<R> applyAsync(IFunction<Long, R> function) {
        isNotNull(function, "function");
        ClientMessage request = AtomicLongApplyCodec.encodeRequest(name, toData(function));
        return invokeOnPartitionAsync(request, APPLY_DECODER);
}

@Override
public <R> R apply(IFunction<Long, R> function) {
         return applyAsync(function).join();
}

Else you get code duplication.

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2016

Nice point, one thing to consider is whether join() method should be pulled up from InternalCompletableFuture to ICompletableFuture (which is returned by IAtomicLong API). It is a convenient wrapper on Future.get and checked exception handling. What do you think?

@pveentjer

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

I agree, but don't know if we should do it already. Ideally we get rid of the ICompletable/InternalCompletableFuture completely and replace it by the java.util.concurrent.CompletableFuture.

In this PR the ClientInvocationFuture implements InternalCompletableFuture btw:
#7939

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2016

Great, so I guess we should wait until #7939 is merged, then de-duplicate client code to match server side implementation (implementation in async methods only, sync methods implemented via *Async.join()) and then check & merge this PR?

@pveentjer

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

Lets merge this one and address the duplication issue when my pr is merged.

@pveentjer

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

👍

@pveentjer pveentjer merged commit 8400f6f into hazelcast:master Apr 19, 2016

1 check passed

default 9164 tests run, 35 skipped, 0 failed.
Details

@vbekiaris vbekiaris deleted the vbekiaris:async-methods-atomiclong-7957 branch Apr 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.