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

Fix origin identification for unserializable exceptions #327

Closed

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented May 16, 2024

For exceptions which were not serializable, we previously only added ErrorId to the ProxyException created for serialization purposes. The original exception that was thrown up through the Pipeline's execution would have no ErrorId, so each block/step that failed due to the exception would get a fresh ErrorId.

Testing done

Submitter checklist

Preview Give feedback

@dwnusbaum dwnusbaum requested a review from jglick May 16, 2024 20:24
@dwnusbaum dwnusbaum requested a review from a team as a code owner May 16, 2024 20:24
@@ -245,4 +247,15 @@ public static class X extends Exception {
assertNotNull(new ErrorAction(unserializable));
assertNotNull(new ErrorAction(cyclic));
}

@Test public void findOriginOfRethrownUnserializableException() throws Throwable {
Copy link
Member Author

@dwnusbaum dwnusbaum May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fails without the changes in src/main because the FlowEndNode is identified as the origin instead.

@dwnusbaum dwnusbaum added the bug label May 16, 2024
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right.

@jglick
Copy link
Member

jglick commented May 16, 2024

…but does not suffice according to #328. Digging into it.

@jglick
Copy link
Member

jglick commented May 16, 2024

Oops, you are making changes here but I am working on changes in the more comprehensive test downstream, so this is all going to clash.

@dwnusbaum
Copy link
Member Author

It's fine, we can close this.

@dwnusbaum dwnusbaum closed this May 16, 2024
jglick added a commit to jglick/workflow-api-plugin that referenced this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants