Refactor part 2 - separate merge from diff iterator [change-log] optimize merge by 20 percent #2884

eden-ohana · 2022-01-23T12:08:41Z

closes #2784
Done - Refactor part 1: separate commit and merge

Refactor part 2:

Separate merge from diff iterator (base, source, dest iterators instead)
Merge iterator is not used anymore by the merge (didn't delete because compare iterator)
Apply is deleted (both merge and commit doesn't use it anymore)
No longer supporting diff summary for merge
Allowing merge with no changes
Optimize seek GE for base iterator

Simplify the diff iterator by removing all extra functionality for merge
Separate compare iterator from diff iterator

guy-har · 2022-01-26T10:22:07Z

pkg/graveler/committed/manager.go

-	defer metaRangeIterator.Close()
-	summary, err = Apply(ctx, mwWriter, metaRangeIterator, diffs, &ApplyOptions{})
+
+	summary, err = Merge(ctx, mwWriter, baseIt, sourceIt, destIt)


Please remove summary if it isn't supported anymore

guy-har · 2022-01-26T12:53:59Z

pkg/graveler/committed/merge.go

+	summary              graveler.DiffSummary
+}
+
+// baseGE returns range from base iterator, which is greater or equal than the given key


The name and description are quite confusing, it doesn't really do as described.
IIUC the function does seekGE but from the current location. This isn't intuitive when using iterators. please document well

guy-har · 2022-01-26T12:54:32Z

pkg/graveler/committed/merge.go

+// baseGE returns value from base iterator, which is greater or equal than the given key
+func (m *merger) baseKeyGE(key graveler.Key) (*graveler.ValueRecord, error) {


Same as in baseRangeGE

guy-har · 2022-01-26T12:56:40Z

pkg/graveler/committed/merge.go

+	if err := m.base.Err(); err != nil {
+		return nil, err
+	}
+	return nil, nil


Suggested change

if err := m.base.Err(); err != nil {

return nil, err

}

return nil, nil

return nil, m.base.Err()

guy-har · 2022-01-26T12:57:15Z

pkg/graveler/committed/merge.go

+	if err := m.base.Err(); err != nil {
+		return nil, err
+	}
+	return nil, nil


Suggested change

if err := m.base.Err(); err != nil {

return nil, err

}

return nil, nil

return nil, m.base.Err()

guy-har · 2022-01-26T14:23:18Z

pkg/graveler/committed/merge.go

+		if baseValue != nil && bytes.Equal(sourceValue.Identity, baseValue.Identity) { // dest deleted this record
+			m.haveSource = m.source.Next()
+		} else {
+			if baseValue != nil && bytes.Equal(sourceValue.Key, baseValue.Key) { // conflict


Suggested change

if baseValue != nil && bytes.Equal(sourceValue.Key, baseValue.Key) { // conflict

if baseValue != nil && bytes.Equal(sourceValue.Key, baseValue.Key) { // deleted by dest and changed by source

guy-har · 2022-01-26T14:23:55Z

pkg/graveler/committed/merge.go

+		if baseValue != nil && bytes.Equal(destValue.Identity, baseValue.Identity) { // source deleted this record
+			m.haveDest = m.dest.Next()
+		} else {
+			if baseValue != nil && bytes.Equal(destValue.Key, baseValue.Key) { // conflict


Suggested change

if baseValue != nil && bytes.Equal(destValue.Key, baseValue.Key) { // conflict

if baseValue != nil && bytes.Equal(destValue.Key, baseValue.Key) { // deleted by source added by dest

guy-har · 2022-01-26T14:26:33Z

pkg/graveler/committed/merge.go

+				return graveler.ErrConflictFound
+			}
+		}
+		// both added the same record


Not precise, there is also the (more common) case of no change
sourceValue.Identity == destValue.Identity == baseValue.Identity

guy-har · 2022-01-26T14:42:04Z

pkg/graveler/committed/merge.go

+			m.haveDest = m.dest.NextRange()
+			return nil
+		} else {
+			return graveler.ErrConflictFound // conflict


No conflicts on ranges

pkg/graveler/committed/merge.go

johnnyaug

Impressive work!
I didn't finish reviewing yet, but since there is already one critical comment I'm publishing my review. Will continue tomorrow

johnnyaug · 2022-01-26T09:04:51Z

nessie/identity_test.go

@@ -53,7 +53,7 @@ func TestIdentity(t *testing.T) {

 			resp, err := client.MergeIntoBranchWithResponse(ctx, repo, branch1, branch2, api.MergeIntoBranchJSONRequestBody{})
 			require.NoError(t, err, "error during merge")
-			require.NotNil(t, resp.JSON400, "merge should fail since there are no changes between the branches")
+			require.Nil(t, resp.JSON400, "allow merge with no changes between the branches")


It's a strange test to check specifically that the 400 error is empty - should probably check that the returned code is not empty (200?)

johnnyaug · 2022-01-26T09:08:04Z

pkg/api/controller_test.go

+	if err != nil {
+		t.Fatal(err)
+	}


Suggested change

if err != nil {

t.Fatal(err)

}

testutil.Must(t, err)

johnnyaug · 2022-01-26T09:11:24Z

pkg/graveler/committed/merge_test.go

 	"fmt"
 	"testing"

-	"github.com/go-test/deep"
+	"github.com/stretchr/testify/assert"
+


organize imports

Not done - there's an empty line here

johnnyaug · 2022-01-26T09:13:21Z

pkg/graveler/committed/merge_test.go

@@ -84,7 +86,6 @@ func Test_merge(t *testing.T) {
 		destRange           []testRange
 		expectedActions     []writeAction
 		conflictExpectedIdx *int
-		expectedSummary     graveler.DiffSummary
 		expectedErr         error


Always nil in this test: not needed anymore

johnnyaug · 2022-01-26T17:08:59Z

pkg/graveler/committed/merge_test.go

@@ -239,8 +235,14 @@ func Test_merge(t *testing.T) {
 			conflictExpectedIdx: nil,
 			expectedActions: []writeAction{
 				{
-					action: actionTypeWriteRange,
-					rng:    committed.Range{ID: "base:k1-k3", MinKey: committed.Key("k1"), MaxKey: committed.Key("k3"), Count: 2, EstimatedSize: 1024},
+					action:   actionTypeWriteRecord,


Does this change mean there was a regression in the optimization? (also applies to other changes in this test)

Yes, it's not a common case so I think we can skip it

johnnyaug · 2022-01-26T18:24:16Z

pkg/graveler/committed/merge.go

+// applyBothRanges applies merging when both source and dest on range header
+func (m *merger) applyBothRanges(sourceRange *Range, destRange *Range) error {
+	switch {
+	case sourceRange.ID == destRange.ID: // source and dest added the same range


// source and dest added the same range

Or, more likely, nobody added anything, right?

johnnyaug · 2022-01-26T18:44:45Z

pkg/graveler/committed/merge.go

+func (m *merger) baseRangeGE(key graveler.Key) (*Range, error) {
+	for {
+		_, baseRange := m.base.Value()
+		if baseRange != nil && bytes.Compare(baseRange.MaxKey, key) >= 0 {


What is the reason for iterating rather than doing SeekGE?
Is it a performance thing?

Naming suggestion: can be called something like "findRangeInBaseIterator" (not good enough, just suggesting a sentiment)

Yes, It's a performance improvement. Instead of doing seekGE from the beginning of base iterator each time, it continues from its current location.

Can you do a benchmark to validate that these two functions really improve something, and document it?

johnnyaug · 2022-01-26T19:02:17Z

pkg/graveler/committed/merge.go

+		if baseRange != nil && (sourceRange.ID == baseRange.ID || destRange.ID == baseRange.ID) {
+			if sourceRange.ID == baseRange.ID { // dest added changes
+				err = m.writeRange(destRange)
+			} else {
+				err = m.writeRange(sourceRange) // source added changes
+			}
+			if err != nil {
+				return err
+			}
+			m.haveSource = m.source.NextRange()
+			m.haveDest = m.dest.NextRange()
+		} else { // enter both ranges
+			m.haveSource = m.source.Next()
+			m.haveDest = m.dest.Next()
+		}


IMO, more readable like this, but it's matter of preference:

Suggested change

if baseRange != nil && (sourceRange.ID == baseRange.ID || destRange.ID == baseRange.ID) {

if sourceRange.ID == baseRange.ID { // dest added changes

err = m.writeRange(destRange)

} else {

err = m.writeRange(sourceRange) // source added changes

}

if err != nil {

return err

}

m.haveSource = m.source.NextRange()

m.haveDest = m.dest.NextRange()

} else { // enter both ranges

m.haveSource = m.source.Next()

m.haveDest = m.dest.Next()

}

if baseRange == nil {

fallthrough;

}

if sourceRange.ID == baseRange.ID {

err = m.writeRange(destRange)

} else if destRange.ID == baseRange.ID {

err = m.writeRange(sourceRange)

} else {

fallthrough;

}

if err != nil {

return err

}

m.haveSource = m.source.NextRange()

m.haveDest = m.dest.NextRange()

johnnyaug · 2022-01-26T19:07:10Z

pkg/graveler/committed/merge.go

+			m.haveSource = m.source.NextRange()
+			return nil
+		}
+		return graveler.ErrConflictFound // conflict - both changed this range


+add test case that covers this code

johnnyaug · 2022-01-26T19:45:01Z

pkg/graveler/committed/merge.go

+			return nil
+		}
+		if baseRange == nil || destRange.ID == baseRange.ID { // source added this range
+			err = m.writeRange(sourceRange)


I think there's a bug here, adding a test case which I think will reflect it (add this to Test_merge).
I hope this is a valid case.
@guy-har, would love your input here as well.

"is it a bug?": { baseRange: []testRange{ { rng: committed.Range{ID: "base:b-c", MinKey: committed.Key("b"), MaxKey: committed.Key("c"), Count: 2, EstimatedSize: 1024}, records: []testValueRecord{{key: "b", identity: "b"}, {key: "c", identity: "c"}}, }, { rng: committed.Range{ID: "base:d-e", MinKey: committed.Key("d"), MaxKey: committed.Key("e"), Count: 2, EstimatedSize: 1024}, records: []testValueRecord{{key: "d", identity: "d"}, {key: "e", identity: "e"}}, }, }, sourceRange: []testRange{ { rng: committed.Range{ID: "source:a-d", MinKey: committed.Key("a"), MaxKey: committed.Key("d"), Count: 2, EstimatedSize: 1024}, records: []testValueRecord{{key: "a", identity: "a"}, {key: "b", identity: "b"}, {key: "d", identity: "d"}}, }, { rng: committed.Range{ID: "source:e", MinKey: committed.Key("e"), MaxKey: committed.Key("e"), Count: 2, EstimatedSize: 1024}, records: []testValueRecord{{key: "e", identity: "e1"}}, }, }, destRange: []testRange{ { rng: committed.Range{ID: "base:b-c", MinKey: committed.Key("b"), MaxKey: committed.Key("c"), Count: 2, EstimatedSize: 1024}, records: []testValueRecord{{key: "b", identity: "b"}, {key: "c", identity: "c"}}, }, { rng: committed.Range{ID: "dest:d-e", MinKey: committed.Key("d"), MaxKey: committed.Key("e"), Count: 2, EstimatedSize: 1024}, records: []testValueRecord{{key: "d", identity: "d1"}, {key: "e", identity: "e"}}, }, }, conflictExpectedIdx: nil, expectedActions: []writeAction{ { action: actionTypeWriteRecord, key: "a", identity: "a", }, { action: actionTypeWriteRecord, key: "b", identity: "b", }, { action: actionTypeWriteRecord, key: "d", identity: "d1", }, { action: actionTypeWriteRecord, key: "e", identity: "e1", }, }, }

I fixed the bug and added this test, thanks!

guy-har

requesting changes because the fast forward optimization for source == base was removed

guy-har · 2022-01-31T09:24:32Z

pkg/graveler/committed/manager.go

-	if source == base {
-		// no changes on source
-		return "", summary, graveler.ErrNoChanges
-	}


Why was this removed? even if it's not an error anymore we could still return destination

You're right, it wasn't supposed to be removed. I added it back without error.

guy-har · 2022-01-31T09:27:45Z

pkg/graveler/graveler.go

@@ -640,7 +640,7 @@ type CommittedManager interface {
 	// Merge applies changes from 'source' to 'destination', relative to a merge base 'base' and
 	// returns the ID of the new metarange and a summary of diffs.  This is similar to a


Suggested change

// returns the ID of the new metarange and a summary of diffs. This is similar to a

// returns the ID of the new metarange. This is similar to a

guy-har · 2022-01-31T12:38:21Z

pkg/graveler/committed/merge_test.go

+			sourceRange: []testRange{
+				{
+					rng:     committed.Range{ID: "source:a-d", MinKey: committed.Key("a"), MaxKey: committed.Key("d"), Count: 2, EstimatedSize: 1024},


I'm confused, the test is called dest change key?

changed its name

guy-har · 2022-01-31T12:42:25Z

pkg/graveler/committed/merge_test.go

-			expectedErr:     graveler.ErrNoChanges,
+			expectedErr: nil,
+		},
+		"source and dest change same range": {


Suggested change

"source and dest change same range": {

"dest removed range and added range after source removed range edges": {

Not sure this is the best name, but it's a bit of a weird test, it's not that both changed the same range. It's that source changes are included in dest ranges.

guy-har · 2022-01-31T12:52:43Z

pkg/graveler/committed/merge_test.go

+				},
+			},
+		},
+		"dest over source left": {


Suggested change

"dest over source left": {

"dest removed all source added": {

guy-har · 2022-01-31T12:53:25Z

pkg/graveler/committed/merge_test.go

+		"same identity different key": {
+			baseRange: []testRange{
+				{
+					rng:     committed.Range{ID: "base:a-d", MinKey: committed.Key("a"), MaxKey: committed.Key("d"), Count: 2, EstimatedSize: 1024},
+					records: []testValueRecord{{key: "a", identity: "a"}, {key: "d", identity: "d"}},
+				},
+			},
+			sourceRange: []testRange{
+				{
+					rng:     committed.Range{ID: "source:a-d", MinKey: committed.Key("a"), MaxKey: committed.Key("d"), Count: 4, EstimatedSize: 1024},
+					records: []testValueRecord{{key: "a", identity: "a"}, {key: "a1", identity: "a"}, {key: "c", identity: "c"}},
+				},
+			},
+			destRange: []testRange{
+				{
+					rng:     committed.Range{ID: "dest:a-d", MinKey: committed.Key("a"), MaxKey: committed.Key("d"), Count: 2, EstimatedSize: 1024},
+					records: []testValueRecord{{key: "a", identity: "a"}, {key: "d", identity: "d"}},
+				},
+			},
+			conflictExpectedIdx: nil,
+			expectedActions: []writeAction{
+				{
+					action:   actionTypeWriteRecord,
+					key:      "a",
+					identity: "a",
+				},
+				{
+					action:   actionTypeWriteRecord,
+					key:      "a1",
+					identity: "a",
+				},
+				{
+					action:   actionTypeWriteRecord,
+					key:      "c",
+					identity: "c",
+				},
+			},


johnnyaug · 2022-02-01T09:51:10Z

pkg/catalog/model.go

@@ -46,7 +46,6 @@ type CommitLog struct {
 }

 type MergeResult struct {


This struct is not really needed anymore

johnnyaug · 2022-02-01T10:00:33Z

pkg/graveler/committed/compare_iterator.go

@@ -18,27 +18,8 @@ type compareIterator struct {
 	err             error
 }

-type mergeIterator struct {


johnnyaug · 2022-02-01T10:11:01Z

pkg/graveler/committed/merge.go

+func (m *merger) baseRangeGE(key graveler.Key) (*Range, error) {
+	for {
+		_, baseRange := m.base.Value()
+		if baseRange != nil && bytes.Compare(baseRange.MaxKey, key) >= 0 {


Can you do a benchmark to validate that these two functions really improve something, and document it?

johnnyaug · 2022-02-01T10:14:51Z

pkg/graveler/committed/merge_test.go

 	"fmt"
 	"testing"

-	"github.com/go-test/deep"
+	"github.com/stretchr/testify/assert"
+


Not done - there's an empty line here

johnnyaug · 2022-02-01T17:59:57Z

pkg/graveler/committed/merge.go

+}
+
+// moveBaseToGERange moves base iterator (from current point) to range which is greater or equal than the given key
+func (m *merger) moveBaseToGERange(key graveler.Key) (*Range, error) {


"Range which is greater or equal than the given key" is not too well-defined: does it mean that this range starts after this key?
I think this method can be changed so that the code calling it will become simpler (see other comments)

johnnyaug · 2022-02-01T18:30:30Z

pkg/graveler/committed/merge.go

+			m.haveSource = m.source.NextRange()
+			return nil
+		}
+		if baseRange == nil || destRange.ID == baseRange.ID { // source added this range


The check destRange.ID == baseRange.ID is irrelevant, and causes an optimization bug. At this point, destRange starts after sourceRange ends. Whether or not baseRange is the same range is not interesting. Only thing that matters is if baseRange intersects with sourceRange or not.

johnnyaug

Thanks for addressing all the comments. Looks very good!
Minor comments + would still love to see a benchmark justifying the existence of getNextGEKey

johnnyaug · 2022-02-06T11:11:32Z

pkg/api/controller.go

-		}
-	}
-	return result
+	writeResponse(w, http.StatusOK, MergeResult{Reference: res}) // optimize returning unknown summary = 0


Comment not clear - can be removed

johnnyaug · 2022-02-06T11:14:48Z

pkg/graveler/committed/merge.go

+	return nil, nil
+}
+
+// getNextOverlappingFromBase moves base iterator (from current point) to the next overlap range with rangeToOverlap


Suggested change

// getNextOverlappingFromBase moves base iterator (from current point) to the next overlap range with rangeToOverlap

// getNextOverlappingFromBase moves base iterator from its current position to the next range overlapping with rangeToOverlap

eden-ohana · 2022-02-06T16:12:50Z

Thanks for the review @guy-har @johnnyaug !
The refactor includes optimization in nextBaseGEkey - instead of moving the base iterator to the next key from the beginning each time, it moves from its current position (without scanning unnecessary ranges).
Before the change the merge took 1.15m. After the optimization it took 56s.

guy-har · 2022-02-06T16:26:09Z

@eden-ohana

GREAT JOB 👍

We really needed this refactor, Thanks!!

refactor - separate merge from dif iterator

8c0b07d

eden-ohana added the exclude-changelog PR description should not be included in next release changelog label Jan 23, 2022

eden-ohana added 3 commits January 23, 2022 17:57

refactor - separate merge from dif iterator

0f0ee50

refactor - separate merge from dif iterator

5fabb17

refactor - separate merge from dif iterator

ddd1be7

eden-ohana requested review from guy-har and johnnyaug January 24, 2022 16:39

guy-har requested changes Jan 26, 2022

View reviewed changes

johnnyaug suggested changes Jan 26, 2022

View reviewed changes

eden-ohana added 7 commits January 27, 2022 16:33

changes from PR

854e1ed

changes from PR

61e53ad

changes from PR

4e6cd9e

changes from PR

a11ba7d

changes from PR

bd975f9

changes from PR

9440d44

changes from PR

29cdb7d

eden-ohana requested review from johnnyaug and guy-har January 30, 2022 15:37

guy-har requested changes Jan 31, 2022

View reviewed changes

eden-ohana added 2 commits January 31, 2022 15:26

changes from PR

24fbd6f

added tests to increase statements coverage

a4d6d0b

eden-ohana requested a review from guy-har January 31, 2022 14:12

johnnyaug reviewed Feb 1, 2022

View reviewed changes

eden-ohana added 2 commits February 2, 2022 21:47

added tests

f1c7f86

bug fix

9c1ef40

eden-ohana requested a review from johnnyaug February 3, 2022 11:27

added comments

18aea3f

johnnyaug approved these changes Feb 6, 2022

View reviewed changes

pr comments

10fa177

eden-ohana changed the title ~~refactor part 2 - separate merge from diff iterator~~ Refactor part 2 - separate merge from diff iterator [change-log] optimize merge by 20 percent Feb 6, 2022

eden-ohana added include-changelog PR description should be included in next release changelog and removed exclude-changelog PR description should not be included in next release changelog labels Feb 6, 2022

eden-ohana merged commit 797c391 into master Feb 6, 2022

eden-ohana deleted the 2784/merge-refactor-2 branch February 6, 2022 16:19

		// baseGE returns value from base iterator, which is greater or equal than the given key
		func (m merger) baseKeyGE(key graveler.Key) (graveler.ValueRecord, error) {

	if baseValue != nil && bytes.Equal(sourceValue.Key, baseValue.Key) { // conflict
	if baseValue != nil && bytes.Equal(sourceValue.Key, baseValue.Key) { // deleted by dest and changed by source

	if baseValue != nil && bytes.Equal(destValue.Key, baseValue.Key) { // conflict
	if baseValue != nil && bytes.Equal(destValue.Key, baseValue.Key) { // deleted by source added by dest

		@@ -640,7 +640,7 @@ type CommittedManager interface {
		// Merge applies changes from 'source' to 'destination', relative to a merge base 'base' and
		// returns the ID of the new metarange and a summary of diffs. This is similar to a

	// returns the ID of the new metarange and a summary of diffs. This is similar to a
	// returns the ID of the new metarange. This is similar to a

	"source and dest change same range": {
	"dest removed range and added range after source removed range edges": {

	"dest over source left": {
	"dest removed all source added": {

		@@ -46,7 +46,6 @@ type CommitLog struct {
		}

		type MergeResult struct {

	// getNextOverlappingFromBase moves base iterator (from current point) to the next overlap range with rangeToOverlap
	// getNextOverlappingFromBase moves base iterator from its current position to the next range overlapping with rangeToOverlap

Refactor part 2 - separate merge from diff iterator [change-log] optimize merge by 20 percent #2884

Refactor part 2 - separate merge from diff iterator [change-log] optimize merge by 20 percent #2884

Conversation

eden-ohana commented Jan 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guy-har left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eden-ohana commented Feb 6, 2022

guy-har commented Feb 6, 2022

GREAT JOB 👍

eden-ohana commented Jan 23, 2022 •

edited

Loading

johnnyaug Jan 26, 2022 •

edited

Loading

johnnyaug Jan 26, 2022 •

edited

Loading

johnnyaug Feb 1, 2022 •

edited

Loading