Use hashicorp/go-memdb instead of truncindex#43624
Conversation
|
Failure is related to the code change Will update |
rumpl
force-pushed
the
feat-memdb-prefix
branch
2 times, most recently
from
190bac0 to
c1c4670
Compare
thaJeztah
added
status/2-code-review
kind/refactor
|
Once again the failure is related to the code, will update |
|
Happy there's tests catching those 👍 |
memdb already knows how to search by prefix so there is no need to keep a separate list of container ids in the truncindex Benchmarks: $ go test -benchmem -run=^$ -count 5 -tags linux -bench ^BenchmarkDBGetByPrefix100$ github.com/docker/docker/container goos: linux goarch: amd64 pkg: github.com/docker/docker/container cpu: Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz BenchmarkDBGetByPrefix100-6 16018 73935 ns/op 33888 B/op 1100 allocs/op BenchmarkDBGetByPrefix100-6 16502 73150 ns/op 33888 B/op 1100 allocs/op BenchmarkDBGetByPrefix100-6 16218 74014 ns/op 33856 B/op 1100 allocs/op BenchmarkDBGetByPrefix100-6 15733 73370 ns/op 33792 B/op 1100 allocs/op BenchmarkDBGetByPrefix100-6 16432 72546 ns/op 33744 B/op 1100 allocs/op PASS ok github.com/docker/docker/container 9.752s $ go test -benchmem -run=^$ -count 5 -tags linux -bench ^BenchmarkTruncIndexGet100$ github.com/docker/docker/pkg/truncindex goos: linux goarch: amd64 pkg: github.com/docker/docker/pkg/truncindex cpu: Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz BenchmarkTruncIndexGet100-6 16862 73732 ns/op 44776 B/op 1173 allocs/op BenchmarkTruncIndexGet100-6 16832 73629 ns/op 45184 B/op 1179 allocs/op BenchmarkTruncIndexGet100-6 17214 73571 ns/op 45160 B/op 1178 allocs/op BenchmarkTruncIndexGet100-6 16113 71680 ns/op 45360 B/op 1182 allocs/op BenchmarkTruncIndexGet100-6 16676 71246 ns/op 45056 B/op 1184 allocs/op PASS ok github.com/docker/docker/pkg/truncindex 9.759s $ go test -benchmem -run=^$ -count 5 -tags linux -bench ^BenchmarkDBGetByPrefix500$ github.com/docker/docker/container goos: linux goarch: amd64 pkg: github.com/docker/docker/container cpu: Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz BenchmarkDBGetByPrefix500-6 1539 753541 ns/op 169381 B/op 5500 allocs/op BenchmarkDBGetByPrefix500-6 1624 749975 ns/op 169458 B/op 5500 allocs/op BenchmarkDBGetByPrefix500-6 1635 761222 ns/op 169298 B/op 5500 allocs/op BenchmarkDBGetByPrefix500-6 1693 727856 ns/op 169297 B/op 5500 allocs/op BenchmarkDBGetByPrefix500-6 1874 710813 ns/op 169570 B/op 5500 allocs/op PASS ok github.com/docker/docker/container 6.711s $ go test -benchmem -run=^$ -count 5 -tags linux -bench ^BenchmarkTruncIndexGet500$ github.com/docker/docker/pkg/truncindex goos: linux goarch: amd64 pkg: github.com/docker/docker/pkg/truncindex cpu: Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz BenchmarkTruncIndexGet500-6 1934 780328 ns/op 224073 B/op 5929 allocs/op BenchmarkTruncIndexGet500-6 1713 713935 ns/op 225011 B/op 5937 allocs/op BenchmarkTruncIndexGet500-6 1780 702847 ns/op 224090 B/op 5943 allocs/op BenchmarkTruncIndexGet500-6 1736 711086 ns/op 224027 B/op 5929 allocs/op BenchmarkTruncIndexGet500-6 2448 508694 ns/op 222322 B/op 5914 allocs/op PASS ok github.com/docker/docker/pkg/truncindex 6.877s Signed-off-by: Djordje Lukic <[email protected]>
| if indexError != nil { | ||
| // When truncindex defines an error type, use that instead | ||
| if indexError == truncindex.ErrNotExist { | ||
| if indexError == container.ErrNotExist { |
There was a problem hiding this comment.
Wondering if (in a follow-up) we should change ErrNotExist to implement errdefs.ErrNotFound , so that we can use errdefs.IsnotFound() for this;
Lines 3 to 6 in 7d4b788
That way we also don't have to export the container.ErrNotExist (and other errors).
|
|
||
| var ( | ||
| // ErrEmptyPrefix is an error returned if the prefix was empty. | ||
| ErrEmptyPrefix = errors.New("Prefix can't be empty") |
There was a problem hiding this comment.
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;
Lines 8 to 11 in 7d4b788
| } | ||
|
|
||
| func (e ErrAmbiguousPrefix) Error() string { | ||
| return fmt.Sprintf("Multiple IDs found with provided prefix: %s", e.prefix) |
There was a problem hiding this comment.
Same (also for a follow-up) should be lowercase, and implement ErrInvalidParameter (basically, none of these are a 500 (server) error.
| Save(*Container) error | ||
| Delete(*Container) error | ||
|
|
||
| GetByPrefix(string) (string, error) |
There was a problem hiding this comment.
We should have a look at the ViewDB and View interfaces;
- both are only implemented by
memDB(so instead ofNewViewDB()returning an interface, it could return a concrete type - Interfaces should generally be defined by the receiver. In this case, the
ViewDBandViewinterfaces are used indaemon, and it'd be better to have the daemon define the interface it expects (so have the daemon specify "I expect these Methods").
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
thaJeztah
added
the
containerd-integration
Use hashicorp/go-memdb instead of truncindex Signed-off-by: CrazyMax <[email protected]>
4 participants
- What I did
Removed
truncindexsince we can get the same feature using memdb. This removes one of the 3 places we keep containers in memory.- How I did it
I changed the ID index in memdb so that it can be searched by a prefix. I also converted the
truncindextests to make sure everything is still working as intended. I only converted some of the benchmarks fromtruncindex,I'm not sure we need the benchmarks for insertion/deletion since those aren't used any more, we are using the same list of containers that are already present in the memdb.We no longer need
patriciaso I removed it from the vendors- How to verify it
The CI should be enough.
Benchmarks:
- A picture of a cute animal (not mandatory but encouraged)