ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Valentin Kulichenko <valentin.kuliche...@gmail.com>
Subject Re: IgniteException throws from asynchronous cache operation.
Date Fri, 20 Mar 2015 18:33:19 GMT
+1 to removing 'extends Future'. We should either follow Java future's
contract or introduce our own w/o extending it.

--
Val

On Fri, Mar 20, 2015 at 9:46 AM, Sergi Vladykin <sergi.vladykin@gmail.com>
wrote:

> Another option is to have additional method getx() which will not throw any
> checked exceptions, but Future API must be implemented correctly to work
> with any utilities written for Future handling. Or yes, to be on a safe
> side we must remove Future extending.
>
> Sergi
>
> 2015-03-20 19:41 GMT+03:00 Dmitriy Setrakyan <dsetrakyan@apache.org>:
>
> > Ok, then remove extending java.util.concurrent.Future then if it bothers
> > everyone. There is no practical reason for extending it anyway. Let's not
> > introduce checked exceptions to Ignite, given that the rest of the
> project
> > is using unchecked exceptions.
> >
> > D.
> >
> > On Fri, Mar 20, 2015 at 9:37 AM, Sergi Vladykin <
> sergi.vladykin@gmail.com>
> > wrote:
> >
> > > I agree with Vladimir. Extending Future and breaking its contract is
> the
> > > same thing as if we were supporting JCache but in incompatible way. Why
> > we
> > > run TCK then?
> > > I'm not sure about collections of different futures but I remember
> myself
> > > writing utility methods like
> > >
> > > getFutureResult(Future fut) {
> > >     try {
> > >           return fut.get();
> > >     }
> > >     catch (ExecutionException e) {
> > >          ... do something
> > >     }
> > >     catch (InterruptedException e) {
> > >
> > >     }
> > > }
> > >
> > > It just will not work. And I don't see any profits from being
> > incompatible
> > > here.
> > >
> > > Sergi
> > >
> > >
> > > 2015-03-20 19:17 GMT+03:00 Dmitriy Setrakyan <dsetrakyan@apache.org>:
> > >
> > > > I actually think we are creating a use case that does not exist.
> Nobody
> > > > will start casting IgniteFuture to Future. I simply do not see a
> reason
> > > for
> > > > it. Has any of you ever had to create a collection of different types
> > of
> > > > futures from different products? It just won't happen.
> > > >
> > > > The reason we extended java.util.concurrent.Future is to make Ignite
> > > users
> > > > work with standard Java APIs. However exception handling in Java
> Future
> > > is
> > > > very painful to work with, and that is exactly the reason why we
> > removed
> > > > all these checked exceptions from it.
> > > >
> > > > D.
> > > >
> > > > On Fri, Mar 20, 2015 at 8:36 AM, Vladimir Ozerov <
> vozerov@gridgain.com
> > >
> > > > wrote:
> > > >
> > > > > + 1 for following future contract.
> > > > >
> > > > > If we have own contract, then why do we extends Future? This only
> > > > confiuses
> > > > > users. If he cast our future to Future, then he will expect checked
> > > > > exceptions and will use try-catch blocks, which will never fire
> > because
> > > > we
> > > > > breakes Future semantics.
> > > > >
> > > > > Furthermore, even new development effrots in Java 8 respects this
> > > > contract.
> > > > > E.g.:
> > > > >
> > > > >
> > > >
> > >
> >
> http://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html
> > > > > They explicitly mention the following: "To simplify usage in most
> > > > contexts,
> > > > > this class also defines methods join()
> > > > > <
> > > > >
> > > >
> > >
> >
> http://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html#join--
> > > > > >
> > > > >  and getNow(T)
> > > > > <
> > > > >
> > > >
> > >
> >
> http://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html#getNow-T-
> > > > > >
> > > > > that
> > > > > instead throw the CompletionException directly in these cases."
> I.e.
> > > > > initial contract is preserved still, but another shortcut methods
> are
> > > > added
> > > > > to provide alternate semantics.
> > > > >
> > > > > So we either should remove "implements Future", or follow it's
> > > contract.
> > > > >
> > > > > On Fri, Mar 20, 2015 at 6:25 PM, Dmitriy Setrakyan <
> > > > dsetrakyan@apache.org>
> > > > > wrote:
> > > > >
> > > > > > Hazelcast has a future now too? I wonder where they got that
idea
> > :)
> > > > > >
> > > > > > Our future overrides the Future.get() method specifically to
> remove
> > > all
> > > > > > checked exceptions from it. This way we make it pretty clear
that
> > it
> > > > will
> > > > > > never throw ExecutionException. I actually like our design.
> > > > > >
> > > > > > D.
> > > > > >
> > > > > > On Fri, Mar 20, 2015 at 8:12 AM, Sergey Evdokimov <
> > > > > sevdokimov@gridgain.com
> > > > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > I've created a ticket:
> > > > > https://issues.apache.org/jira/browse/IGNITE-527
> > > > > > > (Asynchronous cache operations must throw CacheException
> instead
> > of
> > > > > > > IgniteException)
> > > > > > >
> > > > > > > The reason of ExecutionException is separation exception
that
> is
> > > > > > > computation result and other exceptions
> > > > > > > like CancellationException, InterruptedException. I don't
say
> > that
> > > > our
> > > > > > > approach is bad (throwing exception directly without wrapping
> > into
> > > > > > > ExecutionException), but we must honor contract described
in
> > > > > > > java.util.concurrent.Future
> > > > > > > because IgniteFuture extends java.util.concurrent.Future.
> > > > > Theoretically,
> > > > > > > user can pass our IgniteFuture to third party code as a
simple
> > > > > > > java.util.concurrent.Future, third party code will expect
> > > > > > > ExecutionException
> > > > > > > and java.util.concurrent.TimeoutException when call "get(...)".
> > > > > > Hazelcast's
> > > > > > > future has method 'getSafely()' that throws RuntimeException
> > > > directly,
> > > > > > but
> > > > > > > "get()" works according to java.util.concurrent.Future
> contract.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Fri, Mar 20, 2015 at 5:13 PM, Dmitriy Setrakyan <
> > > > > > dsetrakyan@apache.org>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Agree that sync and async counterparts for the same
operation
> > > > should
> > > > > > > > through the same exception. Is it really not the case
now? If
> > > not,
> > > > we
> > > > > > > > should fix it.
> > > > > > > >
> > > > > > > > Disagree about ExecutionException, as the only reason
it was
> > done
> > > > is
> > > > > to
> > > > > > > > support checked exceptions. We have runtime exception,
so we
> > can
> > > > > throw
> > > > > > > the
> > > > > > > > correct exception at all times.
> > > > > > > >
> > > > > > > > D.
> > > > > > > >
> > > > > > > > On Fri, Mar 20, 2015 at 5:32 AM, Sergey Evdokimov
<
> > > > > > > sevdokimov@gridgain.com
> > > > > > > > >
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hello,
> > > > > > > > >
> > > > > > > > > Failed cache operations throw CacheException,
but failed
> > > > > asynchronous
> > > > > > > > > operations throw IgniteException. I think it's
wrong. Same
> > > > > > synchronous
> > > > > > > > and
> > > > > > > > > asynchronous operation must throw same exception.
> > > > > > > > >
> > > > > > > > > BTW. According to contract of
> > java.util.concurrent.Future#get()
> > > > if
> > > > > > > result
> > > > > > > > > of operation is an exception Future#get() should
throw
> > > > > > > ExecutionException
> > > > > > > > > that wrap result exception. We break this contract
and
> throw
> > > > result
> > > > > > > > > exception directly from Future#get(), this may
be cause of
> > > > > problems,
> > > > > > > for
> > > > > > > > > example it's impossible to make out exceptions
that threw
> > > during
> > > > > > > > > computation and other runtime exceptions.
> > > > > > > > > I propose to keep contract of Future#get() as
described in
> > JDK
> > > > > > javadocs
> > > > > > > > and
> > > > > > > > > add our method "take" that throw exception directly
as
> > > > implemented
> > > > > at
> > > > > > > > > Ignite currently.
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message