feat(store): replace all 4 objects maps with gokv.Store abstraction #177

glimchb · 2023-09-12T02:01:05Z

see https://github.com/philippgille/gokv

Signed-off-by: Boris Glimcher Boris.Glimcher@emc.com

codecov · 2023-09-12T02:35:54Z

Codecov Report

Merging #177 (1769edf) into main (dda8c55) will decrease coverage by 6.87%.
The diff coverage is 29.05%.

@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
- Coverage   65.21%   58.34%   -6.87%     
==========================================
  Files          19       20       +1     
  Lines        1374     1575     +201     
==========================================
+ Hits          896      919      +23     
- Misses        434      570     +136     
- Partials       44       86      +42

Files	Coverage Δ
pkg/evpn/evpn.go	`84.84% <100.00%> (-0.66%)`	⬇️
pkg/utils/codec.go	`0.00% <0.00%> (ø)`
pkg/evpn/bridge.go	`56.12% <30.76%> (-15.18%)`	⬇️
pkg/evpn/vrf.go	`54.76% <29.62%> (-13.21%)`	⬇️
pkg/evpn/port.go	`65.38% <30.76%> (-13.97%)`	⬇️
pkg/evpn/svi.go	`48.44% <26.96%> (-16.08%)`	⬇️

artek-koltun · 2023-09-18T13:00:00Z

@glimchb I think that the chosen library gokv have some disadvantages: It is not updated since 2020. It contains only 11 commits, 0 forks and 0 stars. So there is no active community maintaining it. This entails the risks: What if we have a bug there, or we need some additional feature? Who would do that work? Add/fix the functionality on our own? But I suppose we need a library to offload that job
I found gofiber/storage project. It is maintained. It has 211 stars. Furthermore, it has a discord channel, so we can reach out for help if needed. In addition, it supports a broader set of storage backends. Would it be better to depend on that library instead of gokv?

brianroch · 2023-09-18T16:53:38Z

pkg/evpn/port_test.go

+			options := gomap.DefaultOptions
+			options.Codec = utils.ProtoCodec{}
+			store := gomap.NewStore(options)
+			opi := NewServerWithArgs(mockNetlink, store)


Bit of repetition here, could this be extracted to a function? Same across the rest of the test packages.

artek-koltun · 2023-09-19T07:31:49Z

@glimchb I think that the chosen library gokv have some disadvantages: It is not updated since 2020. It contains only 11 commits, 0 forks and 0 stars. So there is no active community maintaining it. This entails the risks: What if we have a bug there, or we need some additional feature? Who would do that work? Add/fix the functionality on our own? But I suppose we need a library to offload that job I found gofiber/storage project. It is maintained. It has 211 stars. Furthermore, it has a discord channel, so we can reach out for help if needed. In addition, it supports a broader set of storage backends. Would it be better to depend on that library instead of gokv?

ok, my oversight. messed up with repository and referred to https://github.com/yifeng01/gokv
https://github.com/philippgille/gokv repo looks ok

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

until gokv implements GetAll() interface added volatile helper for listing of keys Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

artek-koltun · 2023-10-23T06:51:28Z

pkg/evpn/evpn.go

 	Pagination map[string]int
+	ListHelper map[string]bool


@glimchb since ListHelper is used as a set, it is more optimal to use struct{} as a value to save up 1 byte per entry (empty struct occupies 0 bytes).

glimchb force-pushed the gokv branch 2 times, most recently from 89b7d92 to 9c7d643 Compare September 12, 2023 02:34

glimchb force-pushed the gokv branch from 9c7d643 to 251c0b3 Compare September 12, 2023 02:47

glimchb linked an issue Sep 12, 2023 that may be closed by this pull request

feat(store): use gokv pkg to abstract persistant store #178

Closed

glimchb requested review from PWAlessi and brianroch September 13, 2023 13:24

glimchb force-pushed the gokv branch 6 times, most recently from 4c96492 to 4283e5a Compare September 14, 2023 20:32

brianroch reviewed Sep 18, 2023

View reviewed changes

glimchb force-pushed the gokv branch from 4283e5a to c3ec8e7 Compare September 18, 2023 17:13

glimchb added the enhancement New feature or request label Sep 18, 2023

glimchb self-assigned this Sep 19, 2023

glimchb force-pushed the gokv branch 2 times, most recently from 98cfbf6 to 3eb53e3 Compare September 19, 2023 02:13

glimchb changed the title ~~feat(store): use gokv pkg to abstract persistant store~~ feat(store): replace all 4 objects maps with gokv.Store abstraction Oct 17, 2023

glimchb force-pushed the gokv branch from 3eb53e3 to 91f1932 Compare October 17, 2023 23:38

glimchb force-pushed the gokv branch 5 times, most recently from f33dc94 to 2fd5fd6 Compare October 19, 2023 17:49

glimchb added 6 commits October 19, 2023 20:57

feat(store): use proto marshal instead of json

8e7221d

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

feat(store): replace VRFs map with gokv.Store abstraction

170d28b

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

feat(store): replace SVIs map with gokv.Store abstraction

97a17d1

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

feat(store): replace Bridges map with gokv.Store abstraction

464b0b0

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

feat(store): replace Ports map with gokv.Store abstraction

4cd7b06

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

feat(store): replace List methods with gokv.Store abstraction

9eb2859

until gokv implements GetAll() interface added volatile helper for listing of keys Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

glimchb force-pushed the gokv branch from 2fd5fd6 to 1b3f4d2 Compare October 19, 2023 17:58

test(store): fix linter errcheck failure

1769edf

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>

glimchb force-pushed the gokv branch from 1b3f4d2 to 1769edf Compare October 19, 2023 18:01

glimchb marked this pull request as ready for review October 19, 2023 18:08

glimchb requested a review from a team as a code owner October 19, 2023 18:08

glimchb merged commit cdb8b5e into opiproject:main Oct 19, 2023
16 of 18 checks passed

glimchb deleted the gokv branch October 19, 2023 18:09

artek-koltun reviewed Oct 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(store): replace all 4 objects maps with gokv.Store abstraction #177

feat(store): replace all 4 objects maps with gokv.Store abstraction #177

glimchb commented Sep 12, 2023 •

edited

codecov bot commented Sep 12, 2023 •

edited

artek-koltun commented Sep 18, 2023

brianroch Sep 18, 2023 •

edited

artek-koltun commented Sep 19, 2023

artek-koltun Oct 23, 2023

feat(store): replace all 4 objects maps with gokv.Store abstraction #177

feat(store): replace all 4 objects maps with gokv.Store abstraction #177

Conversation

glimchb commented Sep 12, 2023 • edited

codecov bot commented Sep 12, 2023 • edited

Codecov Report

artek-koltun commented Sep 18, 2023

brianroch Sep 18, 2023 • edited

Choose a reason for hiding this comment

artek-koltun commented Sep 19, 2023

artek-koltun Oct 23, 2023

Choose a reason for hiding this comment

glimchb commented Sep 12, 2023 •

edited

codecov bot commented Sep 12, 2023 •

edited

brianroch Sep 18, 2023 •

edited