чт, 15 апр. 2021 г. в 14:26, Ilya Kasnacheev <ilya.kasnach...@gmail.com>:
> Hello! > > > 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. > > I don't think this is feasible at all. > Imagine javadoc for cache.get() method featuring three pages of possible > error codes thrown by this method. > Of course there is no need to write 3 pages of error codes, this makes no sense. I think we can use error ranges here, or, probably, document most important error scenarios. The point here is to try to document errors as much as possible. > Also, updated every two weeks to account for changes in > underlying mechanisms. > > There is still a confusion between "error code for any error in logs" and > "error code for any pair of method & exception". Which one are we > discussing really? > > This is impossible to track or test, but is susceptible for common > error-hiding antipattern where all exceptions are caught in cache.get() and > discarded, and instead a brand new CH-0001 "error in cache.get()" is thrown > to the user. > > Which is certainly not something that anybody wants. > Certainly not. We are talking here about root cause - what is exactly the reason for method call failure. > > Regards, > -- > Ilya Kasnacheev > > > чт, 15 апр. 2021 г. в 13:03, Vladislav Pyatkov <vldpyat...@gmail.com>: > > > 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 > > > -- Best regards, Alexei Scherbakov