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

Simplify AtomicFileWriter and use clearer temporary file names #10058

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Vlatombe
Copy link
Member

  • Uses a more readable name for the temporary file, derived from the target path
  • Streamline the fallback logic for atomic move, and just let the error bubble up otherwise rather than logging then throwing which was leading to duplicate stacktraces in the system logs

Related to #2548 (comment)

See JENKINS-XXXXX.

Testing done

Proposed changelog entries

  • human-readable text

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

* Uses a more readable name for the temporary file, derived from the target path
* Streamline the fallback logic for atomic move, and just let the error bubble up otherwise rather than logging then throwing which was leading to duplicate stacktraces in the system logs

Related to jenkinsci#2548 (comment)
@Vlatombe Vlatombe requested review from jglick and jtnord December 13, 2024 17:07

} catch (AtomicMoveNotSupportedException e) {
// Both files are on the same filesystem, so this should not happen.
LOGGER.log(Level.WARNING, e, () -> "Atomic move " + tmpPath + " → " + destPath + " not supported. falling back to non-atomic move.");
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if this were logged only once per session. Or if subsequent calls to commit just went straight to non-atomic move.

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

Successfully merging this pull request may close these issues.

2 participants