[jira] [Commented] (IGNITE-17602) Trace identifier of an exception can be lost during transferring ReplicaRespone from replica node to ReplicaService

[Alexander Lapin (Jira)]

[ 
https://issues.apache.org/jira/browse/IGNITE-17602?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17601367#comment-17601367
 ] 

Alexander Lapin commented on IGNITE-17602:
--

[~slava.koptilin] LGTM

> Trace identifier of an exception can be lost during transferring 
> ReplicaRespone from replica node to ReplicaService
> ---
>
> Key: IGNITE-17602
> URL: https://issues.apache.org/jira/browse/IGNITE-17602
> Project: Ignite
>  Issue Type: Bug
>Reporter: Vyacheslav Koptilin
>Assignee: Vyacheslav Koptilin
>Priority: Major
>  Labels: ignite-3
>
> Current implementation of processing _ReplicaResponse_ has the following 
> drawbacks:
>  - if sending _ReplicaRequest_ failed for some reason, _ReplicaService_ just 
> throw new exception without preserving traceId if it exists
> {code:java}
> return messagingService.invoke(node.address(), req, 
> RPC_TIMEOUT).handle((response, throwable) -> {
> if (throwable != null) {
> if (throwable instanceof TimeoutException) {
> throw new ReplicationTimeoutException(req.groupId());
> } else if (throwable instanceof PrimaryReplicaMissException) {
> throw (PrimaryReplicaMissException) throwable;
> }
> throw new ReplicationException(req.groupId(), throwable); <- original 
> traceId will be lost
> {code}
>  - _ErrorReplicaResponse_ explicitly transfer error code of an exception, its 
> traceId etc. It seems to me, we can just transfer an exception (our messaging 
> service properly marshal/unmarshal throwables)
>  - _org.apache.ignite.internal.replicator.exception.ExceptionUtils_ does not 
> care about _IgniteInternalChackedException_. For instance _LockException_ 
> (which is checked exception) is transformed to _IgniteInternalException_.
> In addition, _TransactionImpl_ does not properly handle _ExecutionException_:
> {code:java}
> @Override
> public void commit() throws TransactionException {
> try {
> commitAsync().get();
> } catch (ExecutionException e) {
> if (e.getCause() instanceof TransactionException) {
> throw (TransactionException) e.getCause();
> } else {
> throw new TransactionException(e.getCause());
> }
> } catch (Exception e) {
> throw new TransactionException(e);
> }
> }
> {code}
> We should not re-throw _e.getCause()_ because we will lost a stack frame 
> related to _ExecutionException_, and therefore the resulting stack trace will 
> not show a code path/method that called _commit()_.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Commented] (IGNITE-17602) Trace identifier of an exception can be lost during transferring ReplicaRespone from replica node to ReplicaService

[Vyacheslav Koptilin (Jira)]

[ 
https://issues.apache.org/jira/browse/IGNITE-17602?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17598529#comment-17598529
 ] 

Vyacheslav Koptilin commented on IGNITE-17602:
--

Hello [~sanpwc], [~vpyatkov],

Could you please take a look at 
https://github.com/gridgain/apache-ignite-3/pull/56 ?

> Trace identifier of an exception can be lost during transferring 
> ReplicaRespone from replica node to ReplicaService
> ---
>
> Key: IGNITE-17602
> URL: https://issues.apache.org/jira/browse/IGNITE-17602
> Project: Ignite
>  Issue Type: Bug
>Reporter: Vyacheslav Koptilin
>Assignee: Vyacheslav Koptilin
>Priority: Major
>  Labels: ignite-3
>
> Current implementation of processing _ReplicaResponse_ has the following 
> drawbacks:
>  - if sending _ReplicaRequest_ failed for some reason, _ReplicaService_ just 
> throw new exception without preserving traceId if it exists
> {code:java}
> return messagingService.invoke(node.address(), req, 
> RPC_TIMEOUT).handle((response, throwable) -> {
> if (throwable != null) {
> if (throwable instanceof TimeoutException) {
> throw new ReplicationTimeoutException(req.groupId());
> } else if (throwable instanceof PrimaryReplicaMissException) {
> throw (PrimaryReplicaMissException) throwable;
> }
> throw new ReplicationException(req.groupId(), throwable); <- original 
> traceId will be lost
> {code}
>  - _ErrorReplicaResponse_ explicitly transfer error code of an exception, its 
> traceId etc. It seems to me, we can just transfer an exception (our messaging 
> service properly marshal/unmarshal throwables)
>  - _org.apache.ignite.internal.replicator.exception.ExceptionUtils_ does not 
> care about _IgniteInternalChackedException_. For instance _LockException_ 
> (which is checked exception) is transformed to _IgniteInternalException_.
> In addition, _TransactionImpl_ does not properly handle _ExecutionException_:
> {code:java}
> @Override
> public void commit() throws TransactionException {
> try {
> commitAsync().get();
> } catch (ExecutionException e) {
> if (e.getCause() instanceof TransactionException) {
> throw (TransactionException) e.getCause();
> } else {
> throw new TransactionException(e.getCause());
> }
> } catch (Exception e) {
> throw new TransactionException(e);
> }
> }
> {code}
> We should not re-throw _e.getCause()_ because we will lost a stack frame 
> related to _ExecutionException_, and therefore the resulting stack trace will 
> not show a code path/method that called _commit()_.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)