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

GC: Read commit explicitly when it is missing from prescanned commits #8282

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

arielshaqed
Copy link
Contributor

@arielshaqed arielshaqed commented Oct 13, 2024

GC lists all commits, but it can still miss a concurrent commit. When that happens, read it directly from KV.

The issue goes into detail why this is a good idea.

Fixes #8261.

Verified

This commit was signed with the committer’s verified signature.
arielshaqed Ariel Shaqed (Scolnicov)
The issue describes why this is a good idea.

Fixes #8261.
@arielshaqed arielshaqed added GC+ bug Something isn't working include-changelog PR description should be included in next release changelog labels Oct 13, 2024
Copy link

E2E Test Results - Quickstart

11 passed

Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed

Verified

This commit was signed with the committer’s verified signature.
arielshaqed Ariel Shaqed (Scolnicov)
@arielshaqed arielshaqed marked this pull request as ready for review October 13, 2024 11:45
@arielshaqed
Copy link
Contributor Author

Will pull if EITHER reviewer approves, unless you approve but ask me not to pull. THANKS!

Verified

This commit was signed with the committer’s verified signature.
arielshaqed Ariel Shaqed (Scolnicov)
Copy link
Contributor

@Jonathan-Rosenberg Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

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

Great find! THANKS!
LVGTM!
One suggestion to monitor the occurrences of the described race.

type CommitsMap struct {
ctx context.Context
Log logging.Logger
NumMisses int64
Copy link
Contributor

Choose a reason for hiding this comment

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

What about logging this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor Author

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Thanks!

type CommitsMap struct {
ctx context.Context
Log logging.Logger
NumMisses int64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Verified

This commit was signed with the committer’s verified signature.
arielshaqed Ariel Shaqed (Scolnicov)
Observing as a log for now, not a metric, as it is unclear what an
interesting aggregation would be.  OTOH logs are good to understand how this
behaves.
@arielshaqed arielshaqed enabled auto-merge (squash) October 14, 2024 11:05
@arielshaqed arielshaqed merged commit 43a1c9b into master Oct 14, 2024
37 checks passed
@arielshaqed arielshaqed deleted the bug/8261-gc-missing-commit branch October 14, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working GC+ include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: GC: GetGarbageCollectionCommits sometimes fails on missing starting point commit
2 participants