added tree interface #1043

tzahij · 2020-12-10T15:33:00Z

Just one small file
Yoni - please see if it answers your needs.
Others - please see it match our conventions and golang

This change is

johnnyaug

Thanks! This is very much what I was expecting, it will be exteremly helpful.

I left some comments.

The thing I'm missing the most is documentation. What is the functionality provided by each interface and each method. I know it may be clear to you, but it will help others allow.

johnnyaug · 2020-12-10T15:35:17Z

forest/tree/tree.go

+	PartName sstable.ID `json:"part_name"`
+	MaxKey   gr.Key     `json:"max_path"`
+}
+type TreeType struct {


please remove the word "type", it's confusing.
Also from the treePartType

johnnyaug · 2020-12-10T15:38:31Z

forest/tree/tree.go

+}
+
+type TreeWriter interface {
+	HasOpenWriter() bool


It's not really clear to me how the writer works.
Are HasOpenWriter, ForceCloseCurrentPart and IsSplitKey part of the interface, or are they implementation details?

HasOpenWriter and ForceClose are interfaces needed by Apply.
I am not sure they will be needed by others. If Merge is implemented without apply, It will probably need it.
IsSplitKey determines if a key should terminate a part. is probably redundant. So unless someone really wants it - it will be removed

johnnyaug · 2020-12-10T15:38:53Z

forest/tree/tree.go

+	return t.partManger
+}
+
+type TreeRepo interface {


What about GetTree?

Let's discuss it. a Tree is an internal structure of the package. I am not sure it is needed.
There is another one needed, which returns two reduced trees for diff. let's discuss it.

So how can one pass a TreeSlice to use the interface?

forest/tree/tree.go

itaiad200 · 2020-12-10T17:04:09Z

forest/tree/tree.go

+	treeSlice []treePartType
+}
+
+type treesRepo struct {


This is an implementation. Did you mean to create an interface from it?

Will be moved

itaiad200 · 2020-12-10T17:05:37Z

forest/tree/tree.go

+	partManger sstable.Manager
+}
+
+const (


What are these? add documentation if they are part of the interface.

Will be moved to the cache part

forest/tree/tree.go

itaiad200 · 2020-12-10T17:08:04Z

forest/tree/tree.go

+	NewTreeWriter(splitFactor int, closeAsync sstable.BatchWriterCloser) TreeWriter
+	NewScannerFromID(treeID gr.TreeID, start gr.Key) (gr.ValueIterator, error)
+	NewScannerFromTreeParts(treeSlice TreeType, start gr.Key) (gr.ValueIterator, error)
+	GetPartManger() sstable.Manager


Does the tree interface must expose the inner part manager? It's the sort of complication we really need to avoid if possible.

Currently process initialization, and where application parts get trees and sstables is not finalized AFAIK. Once it is - I will happily remove it.

Why is initialisation part of the interface?
Isn't it a one time operation during setup?

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

ozkatz

I think this interface requires a do-over - it expoes some of the implementation details and is pretty hard to understand without seeing how it is being used, so kind of defeating the purpose of having a interface in the first place.

ozkatz · 2020-12-11T12:24:20Z

forest/tree/tree.go

+type TreeWriter interface {
+	HasOpenWriter() bool
+	WriteEntry(record gr.ValueRecord) error
+	ForceCloseCurrentPart()


This very much looks like implementation details, not an interface. What is a current part? it is not mentioned anywhere?

right, removed from interface

ozkatz · 2020-12-11T12:24:52Z

forest/tree/tree.go

+	HasOpenWriter() bool
+	WriteEntry(record gr.ValueRecord) error
+	ForceCloseCurrentPart()
+	IsSplitKey(key gr.Key, rowNum int) bool


Why is this part of a tree writer? Why not decouple these and have a KeySplitter interface?

Removed it from interfaces. currently no need to expose it

ozkatz · 2020-12-11T12:25:45Z

forest/tree/tree.go

+	ForceCloseCurrentPart()
+	IsSplitKey(key gr.Key, rowNum int) bool
+	FlushIterToTree(iter gr.ValueIterator) error
+	SaveTree(reuseTree TreeType) (gr.TreeID, error)


What is a reuseTree? the interface doesn't really explain itself, so at a minimum it requires extensive documentation

Right, added explanation

ozkatz · 2020-12-11T12:26:51Z

forest/tree/tree.go

+
+import (
+	cache "github.com/treeverse/lakefs/forest/cache_map"
+	gr "github.com/treeverse/lakefs/graveler"


let's not alias our own packages, it makes the rest of the implementation harder to read

…ture/tree-interface # Conflicts: # forest/tree/tree.go

johnnyaug · 2020-12-13T08:52:46Z

forest/tree/tree.go

+	MaxKey   gr.Key     `json:"max_path"`
+}
+type TreeSlice struct {
+	treeSlice []TreePart


IMO, if it's a struct including a treeSlice, then the struct type should be called Tree.

after commit

johnnyaug · 2020-12-13T09:30:07Z

forest/tree/tree.go

+	treeSlice []TreePart
+}
+
+type treesRepo struct {


Change "trees-repo" to tree-repo (singular) in all relevant places

after commit

codecov-io · 2020-12-13T10:34:22Z

Codecov Report

Merging #1043 (10f577b) into master (a8dc344) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1043      +/-   ##
==========================================
+ Coverage   46.17%   46.23%   +0.05%     
==========================================
  Files         152      152              
  Lines       12316    12318       +2     
==========================================
+ Hits         5687     5695       +8     
+ Misses       5903     5897       -6     
  Partials      726      726

Impacted Files	Coverage Δ
catalog/mvcc/cataloger_list_entries.go	`85.34% <0.00%> (+0.12%)`	⬆️
config/logger.go	`75.00% <0.00%> (+10.71%)`	⬆️

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 a8dc344...10f577b. Read the comment docs.

…ange once integrated with rest of tree package

johnnyaug · 2020-12-13T11:36:31Z

forest/tree/tree.go

+}
+
+type TreeWriter interface {
+	WriteEntry(record graveler.ValueRecord) error


naming suggestions:
WriteEntry >> WriteValue

FlushIterToTree >> WriteIterator

SaveTree: split to 1. Save() which will save and return tree ID.
and 2. Merge(tree) which will create a new tree on top of an existing one and return a tree id as well.

Good ideas. will do after merge

itaiad200 · 2020-12-13T11:42:22Z

forest/tree/tree.go

+package tree
+
+import (
+	// cache "github.com/treeverse/lakefs/forest/cache_map"


itaiad200 · 2020-12-13T11:44:23Z

forest/tree/tree.go

+	"github.com/treeverse/lakefs/graveler/committed/sstable"
+)
+
+type TreePart struct {


Why do you need json tags?

itaiad200 · 2020-12-13T11:44:51Z

forest/tree/tree.go

+
+type TreePart struct {
+	PartName sstable.ID   `json:"part_name"`
+	MaxKey   graveler.Key `json:"max_path"`


Suggested change

MaxKey graveler.Key `json:"max_path"`

MaxKey graveler.Key `json:"max_key"`

itaiad200 · 2020-12-13T11:46:02Z

forest/tree/tree.go

+	PartName sstable.ID   `json:"part_name"`
+	MaxKey   graveler.Key `json:"max_path"`
+}
+type TreeSlice struct {


Do you need it? Also - name is confusing as it's parts slice.

itaiad200 · 2020-12-13T11:50:58Z

forest/tree/tree.go

+
+type TreeRepo interface {
+	// NewTreeWriter returns a writer that uses the part manager to create a new tree
+	NewTreeWriter(splitFactor int, // average number of keys that we want to stored in a part


Can you use go standard of documentation?
i.e. Everything is about the declaration, not embedded in the args across the lines.

itaiad200 · 2020-12-13T13:02:48Z

forest/tree/tree.go

+	NewTreeWriter(splitFactor int, closeAsync sstable.BatchWriterCloser) TreeWriter
+	NewScannerFromID(treeID gr.TreeID, start gr.Key) (gr.ValueIterator, error)
+	NewScannerFromTreeParts(treeSlice TreeType, start gr.Key) (gr.ValueIterator, error)
+	GetPartManger() sstable.Manager


Why is initialisation part of the interface?
Isn't it a one time operation during setup?

itaiad200 · 2020-12-13T13:03:32Z

forest/tree/tree.go

+}
+
+type TreeWriter interface {
+	WriteEntry(record graveler.ValueRecord) error


itaiad200 · 2020-12-13T13:05:10Z

forest/tree/tree.go

+type TreeRepo interface {
+	// NewTreeWriter returns a writer that uses the part manager to create a new tree
+	NewTreeWriter(splitFactor int, // average number of keys that we want to stored in a part
+		// for more detail, look at "IsSplitKey"


where is 'IsSplitKey' ?

itaiad200 · 2020-12-13T13:06:46Z

forest/tree/tree.go

+
+type TreeRepo interface {
+	// NewTreeWriter returns a writer that uses the part manager to create a new tree
+	NewTreeWriter(splitFactor int, // average number of keys that we want to stored in a part


Is splitFactor changeable between trees? IMO should be a const.

itaiad200 · 2020-12-13T13:08:16Z

forest/tree/tree.go

+	// NewTreeWriter returns a writer that uses the part manager to create a new tree
+	NewTreeWriter(splitFactor int, // average number of keys that we want to stored in a part
+		// for more detail, look at "IsSplitKey"
+		closeAsync sstable.BatchWriterCloser, // component used to close part asynchronously, and wait for all part


You're braking the abstraction layers, why does the caller of the TreeManager need to pass a sstable writer feature?

tzahij · 2020-12-13T14:41:25Z

Initial commit to enable other team members to work against this interface/ will be changed later according to needs

added tree interface

9d08370

tzahij requested review from nopcoder, ozkatz, johnnyaug, arielshaqed and itaiad200 December 10, 2020 15:33

johnnyaug suggested changes Dec 10, 2020

View reviewed changes

itaiad200 requested changes Dec 10, 2020

View reviewed changes

Update forest/tree/tree.go

c356acc

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

ozkatz requested changes Dec 11, 2020

View reviewed changes

tzahij added 3 commits December 12, 2020 22:48

correct after review

19f2632

Merge remote-tracking branch 'origin/feature/tree-interface' into fea…

ad8e838

…ture/tree-interface # Conflicts: # forest/tree/tree.go

correct after review - push again

a56eecd

tzahij requested review from johnnyaug and itaiad200 December 12, 2020 21:06

johnnyaug reviewed Dec 13, 2020

View reviewed changes

fix review comments

d1f1d9f

tzahij added 2 commits December 13, 2020 12:42

had to make some identifiers global because of linter errors. will ch…

c9584e4

…ange once integrated with rest of tree package

had to make some identifiers global because of linter errors. will ch…

10f577b

…ange once integrated with rest of tree package

johnnyaug reviewed Dec 13, 2020

View reviewed changes

itaiad200 requested changes Dec 13, 2020

View reviewed changes

simplied SaveTree interface by splitting it to two interfaces

e688677

tzahij merged commit 8aada34 into master Dec 13, 2020

	MaxKey graveler.Key `json:"max_path"`
	MaxKey graveler.Key `json:"max_key"`

added tree interface #1043

added tree interface #1043

Conversation

tzahij commented Dec 10, 2020 • edited by itaiad200 Loading

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

Choose a reason for hiding this comment

ozkatz 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

codecov-io commented Dec 13, 2020 • edited Loading

Codecov Report

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

tzahij commented Dec 13, 2020

tzahij commented Dec 10, 2020 •

edited by itaiad200

Loading

codecov-io commented Dec 13, 2020 •

edited

Loading