fix: pre-merge is missing commit ID #7750
Conversation
ozkatz
added
include-changelog
E2E Test Results - DynamoDB Local - Local Block Adapter |
| @@ -2777,6 +2777,10 @@ func (g *Graveler) Merge(ctx context.Context, repository *RepositoryRecord, dest | |||
| } | |||
| metadata[MergeStrategyMetadataKey] = mergeStrategyString[mergeStrategy] | |||
| commit.Metadata = metadata | |||
| commitID, err = g.RefManager.AddCommit(ctx, repository, commit) | |||
There was a problem hiding this comment.
I don't understand where CommitID is being passed to the hook..
There was a problem hiding this comment.
I think you'd need to put it on commit for this to work. This may demonstrate that this portion of the code needs a unit test.
There was a problem hiding this comment.
Should just modify TestGraveler_PreMergeHook accordingly
There was a problem hiding this comment.
@ozkatz that's the only blocking comment from my end - it seems like we're not passing the new commitID to the pre merge hook, which was the sole purpose of this change, no?
There was a problem hiding this comment.
I really like the idea of this change! It will allow hooks to present a view consistent with the lakeFS branch. (The implementation won't be trivial, but it will be possible!)
That said, we will need to document exactly what this commit is, and especially is not. I think we can say that the ID identified the commit that will be created at the branch head if the hooks and the merge succeed.
But I think this is clarifies stone strange semantics! The hook can succeed and the merge still fails.
| @@ -2777,6 +2777,10 @@ func (g *Graveler) Merge(ctx context.Context, repository *RepositoryRecord, dest | |||
| } | |||
| metadata[MergeStrategyMetadataKey] = mergeStrategyString[mergeStrategy] | |||
| commit.Metadata = metadata | |||
| commitID, err = g.RefManager.AddCommit(ctx, repository, commit) | |||
There was a problem hiding this comment.
I think you'd need to put it on commit for this to work. This may demonstrate that this portion of the code needs a unit test.
|
Esti is expecting a certain amount of runs to be returned for a commit ID. This obviously changes that number, failing Esti. I don't feel comfortable modifying the Esti test to make it pass because I don't understand what it's actually testing. |
There was a problem hiding this comment.
It's probably just me, but I don't understand how it solves anything. Also - @N-o-Z could you take a look too? I don't remember why we decided to implement it this way
The reason we implemented it this way is to avoid spamming KV with dangling commits for every failed pre-merge hook. This is a relatively common scenario so the risk here is high.
Since we don't have any cleanup mechanism for dangling commits I think we should not introduce this change.
I suggest we first have a cleanup mechanism and then introduce this change
@N-o-Z what's the concern with dangling commits? AFAIK we won't scan those ever, nor during |
As a general rule, we should always make effort the cleanup the DB of unnecessary data. We are making this effort in other flows and I believe we should do the same here. |
| commitID, err = g.RefManager.AddCommit(ctx, repository, commit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("add commit: %w", err) | ||
| } | ||
| if !repository.ReadOnly { | ||
| preRunID = g.hooks.NewRunID() | ||
| err = g.hooks.PreMergeHook(ctx, HookRecord{ |
There was a problem hiding this comment.
Please note that PostMergeHook no longer needs to call UpdateCommitID after this change
…into fix/commit-id-on-merge
|
♻️ PR Preview 8101d0a has been successfully destroyed since this PR has been closed.
🤖 By surge-preview |
Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>
Currently the
"commit_id"field is empty for pre-merge hooks. Other fields (message, author - and even ameta_range_idexist!)This Allows accessing the resulting commit from the hook at the expense of potentially referring to a dangling commit in KV.