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

Derive worker files hash from a self-describing layout #23701

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

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Sep 21, 2024

WorkerFilesHash#getCombinedHash() hashed bytes and strings without also hashing their length. It also reencoded path strings from the internally used UTF-8-in-Latin-1 format to UTF-8 on macOS, which results in key mismatches for the worker across platforms and is unnecessarily slow.

Since this commit already changes the key, also use a slightly more efficient Fingerprint method to hash the args.

`WorkerFilesHash#getCombinedHash()` hashed bytes and strings without also hashing their length. It also reencoded path strings from the internally used UTF-8-in-Latin-1 format to UTF-8 on macOS, which results in key mismatches for the worker across platforms and is unnecessarily slow.

Since this commit already changes the key, also use a slightly more efficient `Fingerprint` method to hash the args.
@fmeum fmeum requested a review from tjgq September 21, 2024 17:42
@fmeum fmeum marked this pull request as ready for review September 21, 2024 17:42
@fmeum fmeum requested a review from a team as a code owner September 21, 2024 17:42
@fmeum fmeum removed the request for review from a team September 21, 2024 17:42
@github-actions github-actions bot added team-Local-Exec Issues and PRs for the Execution (Local) team team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Sep 21, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 21, 2024

@tjgq I didn't encounter a situation in which this is actually a bug (it's probably almost impossible), but found this while trying to understand the tool/non-tool logic for the exec log.

@tjgq tjgq added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally team-Local-Exec Issues and PRs for the Execution (Local) team team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants