Extend merge conflicts resolution [v2] #9552

itaigilo · 2025-09-24T16:11:29Z

Change Description

Allowing an extension for the merge conflict resolution logic.

Will be used by the (Enterprise) compaction conflicts resolver.

nopcoder

added some comment, do not block. we should try and keep the current code work the same out of the box and enable customization without changing how we construct our types.

nopcoder · 2025-09-27T19:27:44Z

cmd/lakefs/cmd/run.go

+		conflictResolvers := catalogfactory.BuildConflictResolvers(blockStore)
+
+		c, err := catalog.New(ctx, catalogConfig, conflictResolvers)


conflict resolver should be option in the calalog.Config, it was made in order not to pass N args to configure the catalog.

As our dear friend Claude likes to say -
"You're absolutely right!"

Updated.

nopcoder · 2025-09-27T19:53:41Z

pkg/catalog/catalog.go

+type EntryConflictResolver interface {
+	// FilterByPath returns true if the path should be considered for conflict resolution.
+	FilterByPath(path string) bool
+
+	// ResolveConflict resolves conflicts between two DBEntry values.
+	// It returns the resolved value, or nil if the conflict cannot be resolved automatically.
+	// Assuming the source and dest have the same key (path).
+	ResolveConflict(ctx context.Context, oCtx graveler.ObjectContext, strategy graveler.MergeStrategy, srcValue, destValue *DBEntry) (*DBEntry, error)
+}


Suggest to enable the catalog users to have access to the default conflict resolver - the current behavior and enable middleware like interface building for the resolve conflict.

It seems that we added FilterByPath just to understand in the graveler level if we configured conflict resolver. If we configure in graveler level a default handler and override it only when we override the handler in the catalog level, it should prevent any extra call to the catalog if we do not provide any implementation in the graveler level.

Following generated code an an example of how to produce a catalog conflict resolver middleware like: (we may not require all the code it is just how http middleware enables you interface/function the same functionality)

type ConflictHandler func( ctx context.Context, oCtx graveler.ObjectContext, strategy graveler.MergeStrategy, srcValue, destValue *DBEntry, ) (*DBEntry, error) // Middleware wraps a handler and returns a new handler. type ConflictMiddleware func(next ConflictHandler) ConflictHandler // Chain applies middlewares (left-to-right) around a base handler. func Chain(base ConflictHandler, mws ...ConflictMiddleware) ConflictHandler { h := base for i := len(mws) - 1; i >= 0; i-- { h = mws[i](h) } return h } // Interface form, if you prefer objects: type ConflictResolver interface { ResolveConflict( ctx context.Context, oCtx graveler.ObjectContext, strategy graveler.MergeStrategy, srcValue, destValue *DBEntry, ) (*DBEntry, error) } // Adapters between interface and function types. func HandlerFromResolver(r ConflictResolver) ConflictHandler { return r.ResolveConflict } func ResolverFromHandler(h ConflictHandler) ConflictResolver { return conflictResolverFunc(h) } type conflictResolverFunc ConflictHandler func (f conflictResolverFunc) ResolveConflict( ctx context.Context, oCtx graveler.ObjectContext, strategy graveler.MergeStrategy, srcValue, destValue *DBEntry, ) (*DBEntry, error) { return ConflictHandler(f)(ctx, oCtx, strategy, srcValue, destValue) }

nopcoder · 2025-09-27T19:56:44Z

pkg/catalog/catalog.go

+type ConflictResolverWrapper struct {
+	ConflictResolver EntryConflictResolver
+}


If we implement middleware/handler like interface at the graveler level - we don't need to create a struct to point to our handler.

nopcoder · 2025-09-27T20:00:27Z

pkg/graveler/committed/manager.go

 }

-func NewCommittedManager(m map[graveler.StorageID]MetaRangeManager, r map[graveler.StorageID]RangeManager, p Params) graveler.CommittedManager {
+func NewCommittedManager(m map[graveler.StorageID]MetaRangeManager, r map[graveler.StorageID]RangeManager, crs []graveler.ConflictResolver, p Params) graveler.CommittedManager {


Suggest that the catalog/graveler/committed will be created and work as the default behavior when created and optionally configured using a method to change the behavior.

Agree, it would simplify some of the code. Also, if we have that functionality, we can change the module we created to accept the committed manager and enrich it if wanted

guy-har · 2025-09-28T15:09:35Z

pkg/graveler/graveler.go

+// ObjectContext holds context information for reading an object
+type ObjectContext struct {
+	StorageID        string
+	StorageNamespace string
+}


Object context is a bit confusing, maybe repositoryContext is more suitable
(It was hard for me to understand why is it OK for the merger that processes many objects to hold the objectContext

Not sure I agree with this -
RepoContext sounds too specific to me, and doesn't relate to the fact that it has what we need for reading an object.
It's also described in the comment...

guy-har · 2025-09-28T15:14:09Z

pkg/graveler/committed/manager.go

+	oCtx := graveler.ObjectContext{
+		StorageID:        string(mctx.storageID),
+		StorageNamespace: string(mctx.ns),
+	}


Why is this strings?
also nit:
merge context can hold the ObjectContext directly

Moved the string conversion to the consumer side,

And merge context is private, and isn't passed to Merge().

guy-har · 2025-09-28T18:06:35Z

pkg/graveler/committed/manager.go

 }

-func NewCommittedManager(m map[graveler.StorageID]MetaRangeManager, r map[graveler.StorageID]RangeManager, p Params) graveler.CommittedManager {
+func NewCommittedManager(m map[graveler.StorageID]MetaRangeManager, r map[graveler.StorageID]RangeManager, crs []graveler.ConflictResolver, p Params) graveler.CommittedManager {


Agree, it would simplify some of the code. Also, if we have that functionality, we can change the module we created to accept the committed manager and enrich it if wanted

guy-har · 2025-09-28T18:13:23Z

pkg/catalog/catalog.go

+	// FilterByPath returns true if the path should be considered for conflict resolution.
+	FilterByPath(path string) bool


Why do we need this? can't Resolve conflict return nil, nil or something if it's not a path it can resolve?

Note that EntryConflictResolver.ResolveConflict() is only called by ConflictResolverWrapper.ResolveConflict().
In order to avoid calling newCatalogEntryFromValueRecord() for each conflicted files (table or not),
I added this filter.

Gotta admit that having only path as a param seems too specific to me -
If you can think of a better option, lmk.

guy-har · 2025-09-28T20:47:13Z

modules/catalog/factory/build.go

+func BuildConflictResolvers(block block.Adapter) []graveler.ConflictResolver {
+	return nil
+}


Why don't we return the StrategyConflictResolver here?

There's the default behavior when the conflict resolvers are nil.
I prefer relying on this default, rather then referring to committed.* here plus maintaining the default in two different places.

OK, my only concern is that if someone decides to add something here in the future he will need to know that he needs to add the StrategyConflictResolver in the chain, otherwise it isn't called anymore

guy-har · 2025-09-28T20:57:17Z

pkg/graveler/committed/merge_test.go

 					metaRangeManagers := make(map[graveler.StorageID]committed.MetaRangeManager)
 					metaRangeManagers[tst.storageID] = metaRangeManager
-					committedManager := committed.NewCommittedManager(metaRangeManagers, rangeManagers, params)
+					resolvers := append(tst.conflictResolvers, &committed.StrategyConflictResolver{})


Why are we adding the StrategyConflictResolver? This is a bit surprising, I wouldn't expect this when calling runMergeTests

Right, old code.
Updated.

guy-har

Looks good, nice work
Added some suggestions, non of them are blockers

guy-har · 2025-09-29T11:04:34Z

modules/catalog/factory/build.go

+func BuildConflictResolvers(block block.Adapter) []graveler.ConflictResolver {
+	return nil
+}


OK, my only concern is that if someone decides to add something here in the future he will need to know that he needs to add the StrategyConflictResolver in the chain, otherwise it isn't called anymore

guy-har · 2025-09-29T11:15:57Z

pkg/catalog/catalog.go

+	if resolvedDBEntry == nil || err != nil {
+		return nil, err
+	}


nit:
prefer this in two separate blocks
That way, it's clear that there are 4 cases (regardless of error)
maybe even a switch case here

switch { case resolvedDBEntry == nil: // Not a conflict the catalog should resolve returnValue = nil case resolvedDBEntry.Path == srcDBEntry.Path: // Resolved to src entry - return src value returnValue = srcValue case resolvedDBEntry.Path == destDBEntry.Path: // Resolved to dest entry - return dest value returnValue = destValue default: // Encode resolved entry to value resolvedEntry := newEntryFromCatalogEntry(*resolvedDBEntry) value, err := EntryToValue(resolvedEntry) if err != nil { return nil, fmt.Errorf("encode resolved entry: %w", err) } returnValue = &graveler.ValueRecord{ Key: srcValue.Key, // srcValue and destValue keys are the same Value: value, } } return returnValue, nil

guy-har · 2025-09-29T11:29:50Z

pkg/graveler/graveler.go

 // MergeStrategy changes from dest or source are automatically overridden in case of a conflict
 type MergeStrategy int

+// ConflictResolver is used to resolve conflicts during a merge operation


Just a suggestion, but I think it's worth mentioning that the resolution is at the object level

Suggested change

// ConflictResolver is used to resolve conflicts during a merge operation

// ConflictResolver is used to resolve conflicting objects during a merge operation

itaigilo added 12 commits September 17, 2025 15:12

Extend merge conflicts resolution

7c38201

Update interfaces

fddbb66

Fix tests

e174b36

Merge branch 'master' into feature/extend-merge-conflicts-resolution

2a69fdb

Fix lint

390ddec

Update structure

12bdefa

Add tests

53184c8

Merge branch 'master' into feature/extend-merge-conflicts-resolution

c4037a7

Rename module

135c8fe

Refactor interfaces

55a0fff

Rename params

d93e514

Cleanup

dca348d

itaigilo requested review from nopcoder, guy-har and a team September 24, 2025 16:11

itaigilo added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached labels Sep 24, 2025

itaigilo mentioned this pull request Sep 24, 2025

Extend merge conflicts resolution #9530

Closed

itaigilo added 12 commits September 24, 2025 17:27

Fix lint

270e4ef

Update mock

d7d5499

Merge branch 'master' into feature/extend-merge-conflicts-resolution-1

3310e91

Refactor to resolvers chain

d1bfaf0

Fix tests

0c1e0ff

Fix lint

7b758be

Fix init

e292f68

Refactor factory

7f48519

Merge branch 'master' into feature/extend-merge-conflicts-resolution-1

7eec959

Cleanup

99cdd64

Cleanup

e896e6b

Cleanup

36f6969

Cleanup

397dbac

itaigilo requested review from guy-har and removed request for guy-har September 26, 2025 14:49

Add path filtering

7b103f9

nopcoder reviewed Sep 27, 2025

View reviewed changes

guy-har reviewed Sep 28, 2025

View reviewed changes

itaigilo added 3 commits September 29, 2025 09:04

Fix PR comments

ec2aafb

Fix CR comments

ca31562

Fix tests

db068f4

itaigilo requested a review from guy-har September 29, 2025 10:06

guy-har approved these changes Sep 29, 2025

View reviewed changes

Fix PR comments

74a1cf1

itaigilo merged commit 08bff00 into master Sep 29, 2025
40 checks passed

itaigilo deleted the feature/extend-merge-conflicts-resolution-1 branch September 29, 2025 13:37

		conflictResolvers := catalogfactory.BuildConflictResolvers(blockStore)

		c, err := catalog.New(ctx, catalogConfig, conflictResolvers)

		// FilterByPath returns true if the path should be considered for conflict resolution.
		FilterByPath(path string) bool

	// ConflictResolver is used to resolve conflicts during a merge operation
	// ConflictResolver is used to resolve conflicting objects during a merge operation

Extend merge conflicts resolution [v2] #9552

Extend merge conflicts resolution [v2] #9552

Uh oh!

Conversation

itaigilo commented Sep 24, 2025

Change Description

Uh oh!

nopcoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guy-har Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guy-har left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

guy-har Sep 28, 2025 •

edited

Loading