Skip to content

Use hashicorp/go-memdb instead of truncindex #43624

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Jul 8, 2022
Merged

Use hashicorp/go-memdb instead of truncindex #43624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions container/view.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const (
memdbContainersTable = "containers"
memdbNamesTable = "names"
memdbIDIndex = "id"
memdbIDIndexPrefix = "id_prefix"
memdbContainerIDIndex = "containerid"
)

Expand All @@ -27,6 +28,24 @@ var (
ErrNameNotReserved = errors.New("name is not reserved")
)

var (
// ErrEmptyPrefix is an error returned if the prefix was empty.
ErrEmptyPrefix = errors.New("Prefix can't be empty")
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah May 23, 2022

Choose a reason for hiding this comment

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

Not new, so ok to do in a follow-up, but this should be lowercase prefix, and ideally, it would return an error that implements errdefs.ErrInvalidParameter;

moby/errdefs/defs.go

Lines 8 to 11 in 7d4b788

// ErrInvalidParameter signals that the user input is invalid
type ErrInvalidParameter interface {
InvalidParameter()
}


// ErrNotExist is returned when ID or its prefix not found in index.
ErrNotExist = errors.New("ID does not exist")
)

// ErrAmbiguousPrefix is returned if the prefix was ambiguous
// (multiple ids for the prefix).
type ErrAmbiguousPrefix struct {
prefix string
}

func (e ErrAmbiguousPrefix) Error() string {
return fmt.Sprintf("Multiple IDs found with provided prefix: %s", e.prefix)
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah May 23, 2022

Choose a reason for hiding this comment

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

Same (also for a follow-up) should be lowercase, and implement ErrInvalidParameter (basically, none of these are a 500 (server) error.

}

// Snapshot is a read only view for Containers. It holds all information necessary to serve container queries in a
// versioned ACID in-memory store.
type Snapshot struct {
Expand Down Expand Up @@ -64,6 +83,8 @@ type ViewDB interface {
Save(*Container) error
Delete(*Container) error

GetByPrefix(string) (string, error)
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah May 23, 2022

Choose a reason for hiding this comment

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

We should have a look at the ViewDB and View interfaces;

  • both are only implemented by memDB (so instead of NewViewDB() returning an interface, it could return a concrete type
  • Interfaces should generally be defined by the receiver. In this case, the ViewDB and View interfaces are used in daemon, and it'd be better to have the daemon define the interface it expects (so have the daemon specify "I expect these Methods").

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah May 23, 2022

Choose a reason for hiding this comment

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

Looking at that; wondering if GetByPrefix() should actually be part of the VIew interface below. For example, daemon.filterByNameIDMatches() takes container.View as an argument (which is daemon.containersReplica.Snapshot()).

That's a bit of a weird construct, as effectively now it's using two separate interfaces to query the same data-source (once through the "read-only" container.View, and once by directly accessing daemon.containersReplica, which is .. not read-only?)

Moving this method to container.View means that filterByNameIDMatches() would only be using the container.View snapshot, not a combination of both.

Copy link
Copy Markdown
Member Author

@rumpl rumpl May 24, 2022

Choose a reason for hiding this comment

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

filterByNameIDMatches indeed takes a View but it doesn't really need it, it only uses it to call view.All(), so in reality it only needs the []Snapshot.

I did initially put GetByPrefix in the View but moved it over to the ViewDB because it made more sense to put it in there. Also, we can and should make GetByPrefix return a *Container rather than just the ID, with this we can remove the call to Get here.

The way I see it, ViewDB will get you *Containers and you use the View to get a snapshot (Snaphot) of the containers.

My thinking is to remove the memory_store.go and merge it in the view.


ReserveName(name, containerID string) error
ReleaseName(name string) error
}
Expand Down Expand Up @@ -131,6 +152,38 @@ func NewViewDB() (ViewDB, error) {
return &memDB{store: store}, nil
}

func (db *memDB) GetByPrefix(s string) (string, error) {
if s == "" {
return "", ErrEmptyPrefix
}
txn := db.store.Txn(false)
iter, err := txn.Get(memdbContainersTable, memdbIDIndexPrefix, s)
if err != nil {
return "", err
}

var (
id string
)

for {
item := iter.Next()
if item == nil {
break
}
if id != "" {
return "", ErrAmbiguousPrefix{prefix: s}
}
id = item.(*Container).ID
}

if id != "" {
return id, nil
}

return "", ErrNotExist
}

// Snapshot provides a consistent read-only View of the database
func (db *memDB) Snapshot() View {
return &memdbView{
Expand Down Expand Up @@ -441,6 +494,20 @@ func (e *containerByIDIndexer) FromArgs(args ...interface{}) ([]byte, error) {
return []byte(arg), nil
}

func (e *containerByIDIndexer) PrefixFromArgs(args ...interface{}) ([]byte, error) {
val, err := e.FromArgs(args...)
if err != nil {
return nil, err
}

// Strip the null terminator, the rest is a prefix
n := len(val)
if n > 0 {
return val[:n-1], nil
}
return val, nil
}

// namesByNameIndexer is used to index container name associations by name.
type namesByNameIndexer struct{}

Expand Down
Loading