Skip to content

[SPARK-53329] Improve exception handling when adding artifacts #52073

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HendrikHuebner
Copy link

@HendrikHuebner HendrikHuebner commented Aug 19, 2025

What changes were proposed in this pull request?

When a user sends multiple artifacts with the addArtifacts API, we process each artifact one at a time on the server-side.

If the server detects the user attempting to modify an artifact (by overwriting an existing artifact of the same path with a different byte sequence), an exception is immediately thrown and artifact addition process is terminated.

Instead, the operation should be idempotent and the server should try to add as many artifacts as possible instead of returning early.

Why are the changes needed?

As explained, if the server encounters an error while adding artifacts it will return immediately. This can be a bit wasteful as the server discards all other artifacts sent over the wire regardless of their own status. Thus, an improvement can be made to process all artifacts, catch any exceptions and rethrow them at the end.

Does this PR introduce any user-facing change?

This PR does not modify the existing API or the return codes. If the above scenario is triggered, the only user facing change is that the server adds as many artifacts as possible. Therefore it should be fully backwards compatible. Additionally, if more than one artifact already existed, its exception is added as a suppressed exception. Currently, these suppressed exceptions are not serialized into the grpc object and sent over the wire, however.

How was this patch tested?

Unit tests and local testing.

Was this patch authored or co-authored using generative AI tooling?

No

@HendrikHuebner HendrikHuebner force-pushed the improve-add-artifact-exceptions branch 6 times, most recently from 60b8723 to adb42bf Compare August 20, 2025 09:06
@HendrikHuebner HendrikHuebner force-pushed the improve-add-artifact-exceptions branch from adb42bf to 5885ce0 Compare August 20, 2025 09:10
@HendrikHuebner HendrikHuebner marked this pull request as ready for review August 20, 2025 11:46
Copy link
Contributor

@vicennial vicennial left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement, LGTM!

One important behaviour to note: Currently, the AddArtifact RPC is retry-friendly as overwriting a file with the same byte sequence is NOT considered as a "true overwrite" and thus, is simply ignored.
This PR does not change the above semantics and AddArtifact will remain retry-friendly.

Currently, these suppressed exceptions are not serialized into the grpc object and sent over the wire, however.

Thanks for the callout! I've filed https://issues.apache.org/jira/browse/SPARK-53350 to track this as a potential improvement/follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note to other reviewers: Hide whitespace changes

Copy link
Author

Choose a reason for hiding this comment

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

@vicennial what do you mean? I saw there was a superfluous whitespace somewhere, I'll remove it before merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

@HendrikHuebner Not related to the code per-se, just the way that github displays the diff.

By default it does not hide whitespace modifications:
image

But if its enabled explicitly, the diff is cleaner and easier to understand what changed:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants