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

DurableExecutorService exceptions #9051

Closed
neilstevenson opened this issue Oct 5, 2016 · 10 comments

Comments

Projects
None yet
6 participants
@neilstevenson
Copy link

commented Oct 5, 2016

If DurableExecutorService.retrieveResult(long) is run from a client for a task that doesn't exist, two exceptions appear on 3.7.1.

In the server logs StaleTaskIdException. This shouldn't be logged to the server if re-thrown to the client.

On the client, ExecutionException. This should be a StaleTaskIdException

@gurbuzali gurbuzali added the Team: Core label Oct 5, 2016

@gurbuzali gurbuzali added this to the 3.8 milestone Oct 5, 2016

@gurbuzali gurbuzali self-assigned this Oct 5, 2016

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

@gurbuzali: are you working on this?

@jerrinot jerrinot assigned jerrinot and unassigned gurbuzali Jan 2, 2017

@mmedenjak

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2017

The observed behaviours are default for all (or at least more than the one listed) service methods. The wrapping with ExecutionException is what you get for all methods that return a Future. This is due to the fact that all invocations are wrapped AbstractInvocationFuture#resolveAndThrowIfException. On the other hand, methods that return a result with blocking do not wrap because of the exception handling (e.g. com.hazelcast.client.proxy.ClientRingbufferProxy#invoke).
And the additional logging is because all operation failures are logged on the instance.

I propose to keep the current situation since it is a big change to change all future returning methods and as I can see ExecutionExceptions are expected - http://docs.hazelcast.org/docs/3.7/manual/html-single/index.html#dealing-with-exceptions.

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2017

@mmedenjak: is there some sort of unpeel-ing in some cases?

@mmedenjak

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2017

We do have ExceptionUtil#peel(java.lang.Throwable) which is used in case of authentication or processing failure on the member side. The client side future wraps the exception during AbstractInvocationFuture.get.

I have tried with some IAtomicLong and Ringbuffer asnyc operations, they all exhibit the described behaviour and wrap in an ExecutionException on Future.get

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2017

in other words: Client violates a contract of DurableExecutorService and other data structures. Is this correct? cc @asimarslan

@Donnerbart

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2017

I think @pveentjer ran into some troubles with that already, when unifying code between member and client. I don't remember though if he just mimicked the existing behavior (two different exceptions) or if he already changed this for some methods.

@mdogan

This comment has been minimized.

Copy link
Member

commented Jan 3, 2017

DurableExecutorService.retrieveResult(long) returns a future and due to Future contract, it can only throw one of InterruptedException, ExecutionException, TimeoutException.
So on both member and client, StaleTaskIdException can't be thrown, if I'm not missing anything.

@mdogan

This comment has been minimized.

Copy link
Member

commented Jan 3, 2017

And as far as I see, DurableExecutorService.retrieveResult(long) doesn't mention anything about StaleTaskIdException.

@mmedenjak

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2017

Seems right, both member and client invocations which return a future wrap in ExecutionException. I don't think there is a reason to change it then.

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Jan 13, 2017

Do not log business exceptions
Exceptions such as StaleSequenceException are part of a normal
flow. They are propagated to user code and there is no reason
why Hazelcast should log them by default.

Fixes hazelcast#9051

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Jan 26, 2017

Do not log business exceptions
Exceptions such as StaleSequenceException are part of a normal
flow. They are propagated to user code and there is no reason
why Hazelcast should log them by default.

Fixes hazelcast#9051

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Jan 31, 2017

Do not log business exceptions
Exceptions such as StaleSequenceException are part of a normal
flow. They are propagated to user code and there is no reason
why Hazelcast should log them by default.

Fixes hazelcast#9051, hazelcast#9781

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Jan 31, 2017

Do not log business exceptions
Exceptions such as StaleSequenceException are part of a normal
flow. They are propagated to user code and there is no reason
why Hazelcast should log them by default.

Fixes hazelcast#9051, hazelcast#9781

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Feb 2, 2017

Do not log business exceptions
Exceptions such as StaleSequenceException are part of a normal
flow. They are propagated to user code and there is no reason
why Hazelcast should log them by default.

Fixes hazelcast#9051, hazelcast#9781

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Feb 2, 2017

Do not log business exceptions
Exceptions such as StaleSequenceException are part of a normal
flow. They are propagated to user code and there is no reason
why Hazelcast should log them by default.

Fixes hazelcast#9051, hazelcast#9781
@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2017

The exception is working as expected (per contract of Future(s))

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.