sanpwc commented on code in PR #4070:
URL: https://github.com/apache/ignite-3/pull/4070#discussion_r1678830382
##########
modules/core/src/test/java/org/apache/ignite/internal/lang/ExceptionUtilsTest.java:
##########
@@ -90,6 +94,28 @@ private static Stream<IgniteCheckedException>
provideIgniteCheckedExceptions() {
);
}
+ @Test
+ void withCauseDoesNotApplyDefaultCodeWhenCodeIsThere() {
Review Comment:
Could you please add few more tests in order to verify that coded exceptions
wrapped with `ExecutionException`, `CompletionException` and
`CancellationException` will also preserve original code. BTW I believe that it
won't work for `CancellationException`.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/table/ItDurableFinishTest.java:
##########
@@ -310,7 +310,7 @@ void testCommitAlreadyAbortedTx() throws
ExecutionException, InterruptedExceptio
Throwable cause =
ExceptionUtils.unwrapCause(transactionException.getCause());
- assertInstanceOf(MismatchingTransactionOutcomeException.class, cause);
+ assertInstanceOf(MismatchingTransactionOutcomeException.class,
cause.getCause());
Review Comment:
That's confusing. What's the cause?
Seems that the problem is deeper.
> How that should work. Almost all our exceptions extend either
IgniteExpception or IgniteCheckedException but placed in internal packages.
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/ItTransactionRecoveryTest.java:
##########
@@ -789,7 +791,8 @@ public void
testPrimaryFailureWhileInflightInProgressAfterFirstResponse() throws
cancelLease(commitPartNode, tblReplicationGrp);
- assertThat(commitFut,
willThrow(MismatchingTransactionOutcomeException.class, 30, SECONDS));
+ TransactionException txEx = assertWillThrow(commitFut,
TransactionException.class, 30, SECONDS);
+ assertThat(txEx.getCause(),
instanceOf(MismatchingTransactionOutcomeException.class));
Review Comment:
Same as above. MismatchingTransactionOutcomeException is a
TransactionException itself. There's no need to wrap it with any other
TransactionException. We may however move
MismatchingTransactionOutcomeException to public API. Same for
IncompatibleSchemaException etc.
Seems that the problem is deeper.
> How that should work. Almost all our exceptions extend either
IgniteExpception or IgniteCheckedException but placed in internal packages.
##########
modules/core/src/main/java/org/apache/ignite/internal/lang/IgniteExceptionMapperUtil.java:
##########
@@ -84,9 +85,29 @@ static void registerMapping(
* @return Public exception.
*/
public static Throwable mapToPublicException(Throwable origin) {
+ return mapToPublicException(origin, ex -> new
IgniteException(INTERNAL_ERR, ex));
+ }
+
+ /**
+ * This method provides a mapping from internal exception to Ignite public
ones.
+ *
+ * <p>The rules of mapping are the following:</p>
+ * <ul>
+ * <li>any instance of {@link Error} is returned as is, except {@link
AssertionError}
+ * that will always be mapped to {@link IgniteException} with the
{@link Common#INTERNAL_ERR} error code.</li>
+ * <li>any instance of {@link IgniteException} or {@link
IgniteCheckedException} is returned as is.</li>
Review Comment:
How that should work. Almost all our exceptions extend either
IgniteExpception or IgniteCheckedException but placed in internal packages.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/MismatchingTransactionOutcomeException.java:
##########
@@ -19,15 +19,13 @@
import static
org.apache.ignite.lang.ErrorGroups.Transactions.TX_UNEXPECTED_STATE_ERR;
-import org.apache.ignite.tx.TransactionException;
-
/**
* The exception is thrown when the transaction result differs from the
intended one.
*
* <p>For example, {@code tx.commit()} is called for a transaction, but the
verification logic decided to abort it instead. The transaction
* will be finished with {@link TxState#ABORTED} and the call to {@code
tx.commit()} will throw this exception.
*/
-public class MismatchingTransactionOutcomeException extends
TransactionException {
+public class MismatchingTransactionOutcomeException extends
TransactionInternalException {
Review Comment:
Semantically it's not internal.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/StaleTransactionOperationException.java:
##########
@@ -21,10 +21,10 @@
import static
org.apache.ignite.lang.ErrorGroups.Transactions.TX_STALE_OPERATION_ERR;
import java.util.UUID;
-import org.apache.ignite.tx.TransactionException;
+import org.apache.ignite.internal.tx.TransactionInternalException;
/** Error that occurs when a stale operation of a completed transaction is
detected. */
-public class StaleTransactionOperationException extends TransactionException {
+public class StaleTransactionOperationException extends
TransactionInternalException {
Review Comment:
Why it's internal? I'd rather move it to public API. Generally I believe
that we should go through all our exceptions case by case and define whether
they are semantically internal or not.
##########
modules/core/src/main/java/org/apache/ignite/internal/lang/IgniteExceptionMapperUtil.java:
##########
@@ -115,7 +134,20 @@ public static Throwable mapToPublicException(Throwable
origin) {
}
// There are no exception mappings for the given exception. This case
should be considered as internal error.
- return new IgniteException(INTERNAL_ERR, origin);
+ return mapCheckingResultIsPublic(origin, unknownProblemMapper);
+ }
+
+ private static Throwable mapCheckingResultIsPublic(Throwable origin,
Function<Throwable, Throwable> unknownProblemMapper) {
+ Throwable result = unknownProblemMapper.apply(origin);
+
+ checkResultIsPublic(result, origin);
+
+ return result;
+ }
+
+ private static void checkResultIsPublic(Throwable mappingResult, Throwable
origin) {
+ assert mappingResult instanceof IgniteException || mappingResult
instanceof IgniteCheckedException :
Review Comment:
I'd also add an assertion that mappingResult is not part of any internal
package.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
