Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow creation of HandlerExecutionExceptions without stacktrace #1901

Closed
dmurat opened this issue Jul 28, 2021 · 5 comments
Closed

Allow creation of HandlerExecutionExceptions without stacktrace #1901

dmurat opened this issue Jul 28, 2021 · 5 comments
Assignees
Labels
Ideal for Contribution Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.
Milestone

Comments

@dmurat
Copy link
Contributor

dmurat commented Jul 28, 2021

Enhancement Description

Since HandlerExecutionExceptionsare essentially carriers for error codes, their stacktrace is not of great value in common scenarios of passing error codes between JVMs. For this reason, I think it would be sensible to add the writableStackTrace parameter to the constructors of HandlerExecutionExceptions. That way a programmer can decide if stacktrace is important enough to pay the penalty of creating it.

Related discussion with a bit more details: https://discuss.axoniq.io/t/handlerexecutionexception-stacktrace-necessary/3453

Current Behaviour

At the moment there is no writableStackTrace parameter in the API of HandlerExecutionException constructors.

Tnx

@dmurat dmurat added the Type: Enhancement Use to signal an issue enhances an already existing feature of the project. label Jul 28, 2021
@smcvb smcvb added Ideal for Contribution Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. labels Jul 29, 2021
@smcvb
Copy link
Member

smcvb commented Jul 29, 2021

Nice addition, @dmurat.
And a great issue to contribute for whoever is willing to!

@dmurat
Copy link
Contributor Author

dmurat commented Aug 3, 2021

I think some things should be clarified a bit. They mainly boil down to the question of whether writableStackTrace should be true by default (new behavior) or not (old behavior):

  • writableStackTrace is false by default:
    We have an original behavior while exposing the writableStackTrace flag to the developer for avoiding stacktrace creation on the handling side. We are also preserving full backward compatibility in behavior. However, the dispatching side is still creating multiple stacktraces in org.axonframework.axonserver.connector.ErrorCode enumeration.
  • writableStackTrace is true by default:
    In this case, stacktraces are not created by default, while the writableStackTrace parameter is still exposed if someone needs it. The default behavior of HandlerExecutionExceptions is more in line with the intended purpose of being a DTO for error codes. To complete the solution, org.axonframework.messaging.RemoteHandlingException and, by extension, org.axonframework.common.AxonException should also provide writableStackTrace parameters. In the case of RemoteHandlingException, the writableStackTrace parameter can be true by default, while for AxonException it should be false. In the end, there are no stacktraces on the handling nor dispatching side.

In my opinion, the second solution is the correct one, but it has a much wider impact. However, it should not cause any problems at the framework or application-level behavior (I doubt many applications depend on the existence of stacktraces). WDYT?

@smcvb
Copy link
Member

smcvb commented Aug 4, 2021

I agree with you, @dmurat.
Option two is more thorough and providing a broader, better-suited solution to the issue at hand.

@dmurat
Copy link
Contributor Author

dmurat commented Aug 7, 2021

Here is the PR proposal: #1905
Tnx

@smcvb smcvb added the Status: In Progress Use to signal this issue is actively worked on. label Aug 9, 2021
@smcvb smcvb added this to the Release 4.6.0 milestone Aug 9, 2021
@smcvb
Copy link
Member

smcvb commented Aug 10, 2021

Closing this issue, as it has been resolved in pull request #1905.
Thanks for drafting up the description and for providing the solution @dmurat!

@smcvb smcvb closed this as completed Aug 10, 2021
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ideal for Contribution Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.
Projects
None yet
Development

No branches or pull requests

2 participants