Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rocks catalog interface #959

Merged
merged 16 commits into from
Nov 29, 2020
Merged

Rocks catalog interface #959

merged 16 commits into from
Nov 29, 2020

Conversation

guy-har
Copy link
Contributor

@guy-har guy-har commented Nov 24, 2020

No description provided.

@codecov-io
Copy link

codecov-io commented Nov 24, 2020

Codecov Report

Merging #959 (ab9f7fd) into master (4c888fa) will decrease coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #959      +/-   ##
==========================================
- Coverage   44.84%   44.58%   -0.27%     
==========================================
  Files         142      142              
  Lines       11448    11547      +99     
==========================================
+ Hits         5134     5148      +14     
- Misses       5673     5757      +84     
- Partials      641      642       +1     
Impacted Files Coverage Δ
export/export_handler.go 20.70% <0.00%> (-4.94%) ⬇️
catalog/mvcc/cataloger_commit.go 80.00% <0.00%> (-2.61%) ⬇️
db/database.go 10.49% <0.00%> (-0.07%) ⬇️
export/export.go 0.00% <0.00%> (ø)
httputil/endpoints.go 0.00% <0.00%> (ø)
stats/sender.go 1.96% <0.00%> (+0.07%) ⬆️
catalog/mvcc/cataloger_merge.go 73.33% <0.00%> (+0.14%) ⬆️
catalog/mvcc/cataloger_export.go 84.31% <0.00%> (+2.79%) ⬆️
catalog/mvcc/cataloger_create_repository.go 62.96% <0.00%> (+3.70%) ⬆️

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 4c888fa...ab9f7fd. Read the comment docs.

catalog/rocks/catalog.go Outdated Show resolved Hide resolved
catalog/rocks/catalog.go Outdated Show resolved Hide resolved
catalog/rocks/catalog.go Show resolved Hide resolved
catalog/rocks/catalog.go Outdated Show resolved Hide resolved
catalog/rocks/catalog.go Outdated Show resolved Hide resolved
@nopcoder nopcoder marked this pull request as ready for review November 26, 2020 13:00
@guy-har guy-har requested review from tzahij and ozkatz November 26, 2020 13:29
catalog/rocks/catalog.go Outdated Show resolved Hide resolved
@nopcoder nopcoder changed the title Feature/rocks catalog interface Rocks catalog interface Nov 29, 2020
@nopcoder nopcoder merged commit 06806c8 into master Nov 29, 2020
@nopcoder nopcoder deleted the feature/rocks-catalog-interface branch November 29, 2020 08:20
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, started these concurrently with pulling.

@@ -73,10 +73,6 @@ type Entry struct {
ETag string
}

func (e *Entry) IsTombstone() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to have different types Entry in mutable and immutable? Because, while the immutables interface never needs a tombstone, AFAICT the mutables interface must have one if we want to layer mutables on top of immutables. (The only alternative I can see is to have mutables know about immutables and overlay for itself… which just ends up making a different internal "entry" type there.)

// StorageNamespace is the URI to the storage location
StorageNamespace string

// RepositoryID is an identifier for a repo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// RepositoryID is an identifier for a repo
// RepositoryID is an identifier for a repo

I think we enforce allowed characters here.

RepositoryID string

// Path represents a logical path for an entry
Path string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it start with /?

// Path represents a logical path for an entry
Path string

// Ref could be a commit ID, a branch name, a Tag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Ref could be a commit ID, a branch name, a Tag
// Ref locates a tree at a certain point in time. It may be a commit ID, a branch name, or a tag

// Ref could be a commit ID, a branch name, a Tag
Ref string

// TagID represents a named tag pointing at a commit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it immutable? (I'd expect half, just like in git)


// Entry represents metadata or a given object (modified date, physical address, etc)
type Entry struct {
LastModified time.Time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LastModified time.Time
CreationDate time.Time

Can we ever change an entry?

// Repository represents repository metadata
type Repository struct {
StorageNamespace StorageNamespace
CreationDate time.Time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should decide if we want to call it a "date". I prefer CreatedAt or CreationTime; preferably the former because it does not repeat the type.


// Interfaces
type Catalog interface {
// GetEntry returns entry from repository / reference by path, nil entry is a valid value for tombstone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Tombstone" still here.

// Interfaces
type Catalog interface {
// GetEntry returns entry from repository / reference by path, nil entry is a valid value for tombstone
// returns error if entry does not exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's specify which error (we had weirdness on this in the MVCC implementation).

catalog/rocks/catalog.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants