Make objects deleted in the same branch that created them continue to show up in older commits (#997) #1000

tzahij · 2020-12-05T10:27:33Z

fixed in regular and by prefix listing

…verse/lakeFS into 945-sstable-tree-interfaces

…terfaces

Co-authored-by: itaiad200 <itaiad200@gmail.com>

…e/lakeFS into feature/rocks-catalog-interface

…interfaces

945 sstable interface

# Conflicts: # forest/sstable/sstable.go

…n a later commit

arielshaqed

Please rebase your branch on master. Right now there are 35 commits in this branch, which makes it hard to review each commit separately.

I do not understand this commit well enough to approve it. OTOH, I am therefore unequally unqualified to say that it is safe and does not break other behaviours.

catalog/mvcc/cataloger_delete_entry_test.go

arielshaqed · 2020-12-05T10:47:45Z

catalog/mvcc/cataloger_list_entries.go

 	}
 	return unionMap
 }

-func selectSingleBranch(branchID int64, isBaseBranch bool, branchBatchSize int, topCommitID CommitID, prefixLen int, endOfPrefixRange string) sq.SelectBuilder {
-	rawSelect := sq.Select("branch_id", "min_commit").
+func selectSingleBranch(branchID int64, branchBatchSize int, topCommitID CommitID, prefixLen int, endOfPrefixRange string) sq.SelectBuilder {


Please document the function with these parameters. The action of this function is confusing.

arielshaqed · 2020-12-05T10:49:30Z

catalog/mvcc/cataloger_list_entries.go

 	}
 	return unionMap
 }

-func selectSingleBranch(branchID int64, isBaseBranch bool, branchBatchSize int, topCommitID CommitID, prefixLen int, endOfPrefixRange string) sq.SelectBuilder {
-	rawSelect := sq.Select("branch_id", "min_commit").
+func selectSingleBranch(branchID int64, branchBatchSize int, topCommitID CommitID, prefixLen int, endOfPrefixRange string) sq.SelectBuilder {


Also I don't understand the action of parameter isBaseBranch, why we needed it, and why we no longer do. I don't want to expose some bug that caused that parameter, and it is getting hard for me to track.
I can see that isBaseBranch used to change the behaviour of max_commit. Is an unchanged leftover from changing the min_commit and max_commit values?

Fixing the bug here made the code simpler. I found I can have the same behavior on max_commit in the base branch, and all its lineage. That is the reason I happily removed "isBaseBranch". This change has nothing to do with the change to minCommit behavior. I just got smarter :-). As for the other parameters - Which one needs clarification?

Co-authored-by: arielshaqed <ariels@treeverse.io>

arielshaqed · 2020-12-05T13:49:30Z

Also, can we retitle this? AFAICT it does not "erase the object from all commits from master", see my comments on #997 where I reproduced. The objects are deleted from the listings, but they still exist. We should avoid misinformation.

nopcoder

The code doesn't pass the test

nopcoder · 2020-12-05T14:05:27Z

catalog/mvcc/cataloger_delete_entry_test.go

@@ -120,3 +122,94 @@ func testDeleteEntryCommitAndExpectNotFound(t *testing.T, ctx context.Context, c
 		t.Fatalf("DeleteEntry() get entry err = %s, want = %s", err, wantErr)
 	}
 }
+
+func TestCataloger_DeleteEntryAndCheckItRemainsInCommits(t *testing.T) {


Think you can leave the second test as it cover this one - but it is just me ;)

The previous test is somewhat different, and it covers list by prefix (where a separate bug was fixed)

nopcoder · 2020-12-05T16:35:45Z

catalog/mvcc/views.go

 		fmt.Sprintf("min_commit != %d AS is_committed", MinCommitUncommittedIndicator),
 		"max_commit = 0 AS is_tombstone",
 		"ctid AS entry_ctid\n",
-		fmt.Sprintf("max_commit < %d AS is_deleted", MaxCommitID)).
+		fmt.Sprintf("max_commit < %d AS is_deleted", actualCommit)).


Use the Column syntax - ex: .Column("min_commit != ? AS is_committed", MinCommitUncommittedIndicator)

Thanks, Fixed them all

codecov-io · 2020-12-05T21:35:49Z

Codecov Report

Merging #1000 (db47811) into master (b1b3858) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1000      +/-   ##
==========================================
+ Coverage   44.26%   44.27%   +0.01%     
==========================================
  Files         140      140              
  Lines       11570    11570              
==========================================
+ Hits         5121     5123       +2     
+ Misses       5799     5797       -2     
  Partials      650      650

Impacted Files	Coverage Δ
catalog/mvcc/cataloger_list_entries.go	`85.21% <100.00%> (-0.32%)`	⬇️
catalog/mvcc/views.go	`100.00% <100.00%> (+2.98%)`	⬆️
catalog/mvcc/cataloger_create_branch.go	`84.90% <0.00%> (-3.78%)`	⬇️
catalog/mvcc/cataloger_create_entry.go	`100.00% <0.00%> (+5.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1b3858...db47811. Read the comment docs.

arielshaqed · 2020-12-06T19:01:39Z

This comment (missed in the commits) needs to be addressed immediately:

Also, can we retitle this? AFAICT it does not "erase the object from all commits from master", see my comments on #997 where I reproduced. The objects are deleted from the listings, but they still exist. We should avoid misinformation.

The title of this PR is simply the title of issue #997. That issue was titled without a root-cause analysis, since it came from outside (thanks again, @tsykes-2020, for a really good catch!). To limit further fallout I am changing the PR title unilaterally. @tzahij and others: please feel free to correct the title further as needed!

itaiad200 and others added 30 commits November 22, 2020 16:14

SSTable interface for committed data

b43ebf8

Remove unnecessary funcs from iterator

703c4b6

added test for merge with zero changes

84bd269

initial commit of tree

0a4d19b

Change iterator to match the existing scanners interfaces

d2fa10a

Merge branch '945-sstable-tree-interfaces' of https://github.com/tree…

1ed15b2

…verse/lakeFS into 945-sstable-tree-interfaces

initial commit of tree

2bce373

initial commit of tree

71ac20f

initial commit for rocks catalog interface

8550a6d

sstable initial implementation without repo seperation

d32c46c

Merge remote-tracking branch 'origin/master' into 945-sstable-tree-in…

3b721d0

…terfaces

initial catalog interface

8931903

Merge branch 'master' into feature/rocks-catalog-interface

99bdab8

initial catalog interface

acca7d9

Merge branch 'master' into feature/rocks-catalog-interface

f711b30

initial catalog interface

9feb2fd

Update catalog/rocks/catalog.go

9505288

Co-authored-by: itaiad200 <itaiad200@gmail.com>

iterator interface

8eec272

diff type comment

864c5c8

Merge branch 'feature/rocks-catalog-interface' of github.com:treevers…

19bfe19

…e/lakeFS into feature/rocks-catalog-interface

continue talking about StagingManager

0834c13

initial commit of tree

a5cad17

Merge branch 'master' into 945-sstable-tree-interfaces

238838a

Merge branch 'feature/rocks-catalog-interface' into 945-sstable-tree-…

3162279

…interfaces

just save changes - initial

6da61db

working on apply

36df291

working on apply

9768b34

working on apply

d30d1d5

Merge remote-tracking branch 'origin/master' into 945-sstable-interface

8ff22f1

sstable interface

e486488

itaiad200 and others added 5 commits November 29, 2020 13:32

changes

ab24d5b

Merge pull request #969 from treeverse/945-sstable-interface

1cb8a36

945 sstable interface

Merge remote-tracking branch 'origin/master'

a773731

# Conflicts: # forest/sstable/sstable.go

Merge remote-tracking branch 'origin/master'

12c1e04

fix to entry disappearing from a commit listing when it was deleted i…

6c4b572

…n a later commit

tzahij requested review from nopcoder and arielshaqed December 5, 2020 10:27

arielshaqed reviewed Dec 5, 2020

View reviewed changes

Update catalog/mvcc/cataloger_delete_entry_test.go

71a41d3

Co-authored-by: arielshaqed <ariels@treeverse.io>

tzahij linked an issue Dec 5, 2020 that may be closed by this pull request

Committing an object deletion to master erases object from all commits to master #997

Closed

nopcoder suggested changes Dec 5, 2020

View reviewed changes

compare with get entry with catalog entry not found

b40208a

changes after review

db47811

tzahij requested review from arielshaqed and nopcoder December 6, 2020 12:36

nopcoder approved these changes Dec 6, 2020

View reviewed changes

tzahij merged commit 99e89ec into master Dec 6, 2020

arielshaqed changed the title ~~Committing an object deletion to master erases object from all commits to master - #997~~ Make objects deleted in the same branch that created them continue to show up in older commits (#997) Dec 6, 2020

nopcoder deleted the bugfix/issue-997 branch February 4, 2021 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make objects deleted in the same branch that created them continue to show up in older commits (#997) #1000

Make objects deleted in the same branch that created them continue to show up in older commits (#997) #1000

tzahij commented Dec 5, 2020

arielshaqed left a comment

arielshaqed Dec 5, 2020

arielshaqed Dec 5, 2020

tzahij Dec 6, 2020

arielshaqed commented Dec 5, 2020

nopcoder left a comment

nopcoder Dec 5, 2020

tzahij Dec 6, 2020

nopcoder Dec 5, 2020

tzahij Dec 6, 2020

codecov-io commented Dec 5, 2020 •

edited

Loading

arielshaqed commented Dec 6, 2020

Make objects deleted in the same branch that created them continue to show up in older commits (#997) #1000

Make objects deleted in the same branch that created them continue to show up in older commits (#997) #1000

Conversation

tzahij commented Dec 5, 2020

arielshaqed left a comment

Choose a reason for hiding this comment

arielshaqed Dec 5, 2020

Choose a reason for hiding this comment

arielshaqed Dec 5, 2020

Choose a reason for hiding this comment

tzahij Dec 6, 2020

Choose a reason for hiding this comment

arielshaqed commented Dec 5, 2020

nopcoder left a comment

Choose a reason for hiding this comment

nopcoder Dec 5, 2020

Choose a reason for hiding this comment

tzahij Dec 6, 2020

Choose a reason for hiding this comment

nopcoder Dec 5, 2020

Choose a reason for hiding this comment

tzahij Dec 6, 2020

Choose a reason for hiding this comment

codecov-io commented Dec 5, 2020 • edited Loading

Codecov Report

arielshaqed commented Dec 6, 2020

codecov-io commented Dec 5, 2020 •

edited

Loading