Hi Alexei,
> Each public method *must *have a section in the javadoc with a list of
all possible error codes for this method.
I consider it is redundant, because any public exception can be thrown from
public API.
If it not happens today, it may change tomorrow: one will be removed,
another one will be added.
>Nested exceptions are not forbidden to use. They can provide additional
details on the error for debug purposes
I see another issue, which is in the Ignite 2.x, but not attend here. We
can have a deep nested exception and use it for handling.
In the result, all time when we are handling an exception we use
pattern like this:
try{
...
}
catch (Exception e) {
if (X.hasCause(e, TimeoutException.class))
throw e;
if (X.hasCause(e, ConnectException.class, EOFException.class))
continue;
if (X.hasCause(e, InterruptedException.class))
return false;
}
If we have a pant to make only one exception to the client side, we can
also do it for an internal exception.
On Wed, Apr 14, 2021 at 11:42 AM Alexei Scherbakov <
alexey.scherbak...@gmail.com> wrote:
> Alexey,
>
> ср, 14 апр. 2021 г. в 01:52, Alexey Kukushkin <kukushkinale...@gmail.com>:
>
> > Just some points looking questionable to me, although I realize the error
> > handling style may be very opinionated:
> >
> > - Would it make sense splitting the proposed composite error code
> > (TBL-0001) into separate numeric code (0001) and scope/category
> ("TBL")
> > to
> > avoid parsing the code when an error handler needs to analyze only the
> > category or the code?
> >
>
> TBL-0001 is a *string representation* of the error. It is built from 2
> byte scope id (mapped to name TBL) and 2 byte number (0001). Both
> internally packed in int. No any kind of parsing will be necessary to read
> scope/category.
>
>
> > - "*The cause - short string description of an issue, readable by
> > user.*".
> > This terminology sounds confusing to me since that "cause" sounds like
> > Java
> > Throwable's Message to me and the "Cause" is a lower level exception.
> >
>
> The string describes the cause of error, so the name. I'm ok to rename it
> to a message. It will be stored in IgniteException.message field anyway.
>
>
> > - "*The action - steps for a user to resolve error...*". The action is
> > very important but do we want to make it part of the IgniteException?
> I
> > do
> > not think the recovery action text should be part of the exception.
> > IgniteException may include a URL pointing to the corresponding
> > documentation - this is discussable.
> >
>
> This will not be the part of the exception. A user should visit the
> documentation page and read the action section by corresponding error code.
>
>
> > - "*All public methods throw only unchecked IgniteException*" - I
> assume
> > we still respect JCache specification and prefer using standard Java
> > exception to signal about invalid parameters.
> >
>
> Using standard java exceptions whenever possible makes sense.
>
>
> > - Why we do not follow the "classic" structured exception handling
> > practices in Ignite:
> >
>
> Ignite 3 will be multi language, and other languages use other error
> processing models. SQL for example uses error codes.
> The single exception approach simplifies and unifies error handling across
> platforms for me.
>
>
> > - Why do we not allow using checked exceptions? It seems to me
> > sometimes forcing the user to handle an error serves as a hint and
> > thus
> > improves usability. For example, handling an optimistic/pessimistic
> > transaction conflict/deadlock. Or handling a timeout for operations
> > with
> > timeouts.
> >
>
> A valid point. Checked exceptions must be used for whose methods, where
> error handling is enforced, for example tx optimistic failure.
> Such errors will also have corresponding error codes.
>
>
> > - Why single public IgniteException and no exception hierarchy?
> Java
> > is optimized for structured exception handling instead of using
> > IF-ELSE to
> > analyze the codes.
> >
>
> Exception hierarchy is not required when using error codes and applicable
> only to java API, so I would avoid spending efforts on it.
>
>
> > - Why no nested exceptions? Sometimes an error handler is
> interested
> > only in high level exceptions (like Invalid Configuration) and
> > sometimes
> > details are needed (like specific configuration parser exceptions).
> >
>
> Nested exceptions are not forbidden to use. They can provide additional
> details on the error for debug purposes, but not strictly required, because
> error code + message should provide enough information to the user.
>
>
> > - For async methods returning a Future we may have a universal rule on
> > how to handle exceptions. For example, we may specify that any async
> > method
> > can throw only invalid argument exceptions. All other errors are
> > reported
> > via the exceptionally(IgniteException -> {}) callback even if the
> async
> > method was executed synchronously.
> >
>
> This is ok to me.
>
>
> >
> >
> > вт, 13 апр. 2021 г. в 12:08, Alexei Scherbakov <
> > alexey.scherbak...@gmail.com
> > >:
> >
> > > Igniters,
> > >
> > > I would like to start the discussion about error handling in Ignite 3
> and
> > > how we can improve it compared to Ignite 2.
> > >
> > > The error handling in Ignite 2 was not very good because of generic
> > > CacheException thrown on almost any occasion, having deeply nested root
> > > cause and often containing no useful information on further steps to
> fix
> > > the issue.
> > >
> > > I aim to fix it by introducing some rules on error handling.
> > >
> > > *Public exception structure.*
> > >
> > > A public exception must have an error code, a cause, and an action.
> > >
> > > * The code - the combination of 2 byte scope id and 2 byte error number
> > > within the module. This allows up to 2^16 errors for each scope, which
> > > should be enough. The error code string representation can look like
> > > RFT-0001 or TBL-0001
> > > * The cause - short string description of an issue, readable by user.
> > This
> > > can have dynamic parameters depending on the error type for better user
> > > experience, like "Can't write a snapshot, no space left on device {0}"
> > > * The action - steps for a user to resolve error situation described in
> > the
> > > documentation in the corresponding error section, for example "Clean up
> > > disk space and retry the operation".
> > >
> > > Common errors should have their own scope, for example IGN-0001
> > >
> > > All public methods throw only unchecked
> > > org.apache.ignite.lang.IgniteException containing aforementioned
> fields.
> > > Each public method must have a section in the javadoc with a list of
> all
> > > possible error codes for this method.
> > >
> > > A good example with similar structure can be found here [1]
> > >
> > > *Async timeouts.*
> > >
> > > Because almost all API methods in Ignite 3 are async, they all will
> have
> > a
> > > configurable default timeout and can complete with timeout error if a
> > > computation is not finished in time, for example if a response has not
> > been
> > > yet received.
> > > I suggest to complete the async op future with TimeoutException in this
> > > case to make it on par with synchronous execution using future.get,
> which
> > > will throw java.util.concurrent.TimeoutException on timeout.
> > > For reference, see java.util.concurrent.CompletableFuture#orTimeout
> > > No special error code should be used for this scenario.
> > >
> > > *Internal exceptions hierarchy.*
> > >
> > > All internal exceptions should extend
> > > org.apache.ignite.internal.lang.IgniteInternalException for checked
> > > exceptions and
> > > org.apache.ignite.internal.lang.IgniteInternalCheckedException for
> > > unchecked exceptions.
> > >
> > > Thoughts ?
> > >
> > > [1] https://docs.oracle.com/cd/B10501_01/server.920/a96525/preface.htm
> > >
> > > --
> > >
> > > Best regards,
> > > Alexei Scherbakov
> > >
> >
> >
> > --
> > Best regards,
> > Alexey
> >
>
>
> --
>
> Best regards,
> Alexei Scherbakov
>
--
Vladislav Pyatkov
