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

Only insert executable into cache once it starts #528

Merged
merged 2 commits into from
Aug 13, 2024
Merged

Conversation

dmah42
Copy link
Contributor

@dmah42 dmah42 commented Aug 13, 2024

The observability hooks are created in Executable::new so we shouldn't miss any messages. This ensures that we only add started executables to the cache, which means that we don't end up with orphan executables in the cache.

The observability hooks are created in `Executable::new` so we shouldn't
miss any messages.  This ensures that we only add started executables to
the cache, which means that we don't end up with orphan executables in
the cache.
Copy link
Contributor

@future-highway future-highway left a comment

Choose a reason for hiding this comment

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

Looks good.

There is a comment about changing contains_key to try_insert in the future, which seems like it could accidently revert this change, so maybe remove that comment too?

@future-highway
Copy link
Contributor

future-highway commented Aug 13, 2024

I don't know how to unapprove...give me a sec...might be a bug

hidden lines through me off...looks good

@dmah42
Copy link
Contributor Author

dmah42 commented Aug 13, 2024

try_insert still isn't in stable. I came here to do that change :)

@dmah42 dmah42 merged commit e789441 into main Aug 13, 2024
4 checks passed
@dmah42 dmah42 deleted the executable_insert branch August 13, 2024 15:58
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