-
Notifications
You must be signed in to change notification settings - Fork 92
Remove unnecessary methods from Store2, and moves handling of protobuf to store
#1120
Conversation
pkg/config/store/queue.go
Outdated
func (q *eventQueue) run() { | ||
loop: | ||
for { | ||
select { | ||
case <-q.ctx.Done(): | ||
fmt.Printf("done0\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt? use glog.Info ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, it was for my debugging. Removed.
pkg/config/store/store2.go
Outdated
// Init initializes the connection with the storage backend. This uses "kinds" | ||
// for the mapping from the kind's name and its structure in protobuf. | ||
// The connection will be closed after ctx is done. | ||
func (s *Store2) Init(ctx context.Context, kinds map[string]proto.Message) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make Store2 an interface.
pkg/config/store/convert.go
Outdated
|
||
func convert(spec map[string]interface{}, target proto.Message) error { | ||
// This is inefficient; convert to a protobuf message through JSON. | ||
// TODO: use reflect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove comment.
reflect will not work, it will not let us use ENUM symbolic constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/config/store/store2.go
Outdated
backend Store2Backend | ||
} | ||
|
||
// NewStore2 creates a new Store2 instance with the specified backend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need an abstract constructor that takes NewStore2(storeURL)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Codecov Report
@@ Coverage Diff @@
## master #1120 +/- ##
=========================================
Coverage ? 88.57%
=========================================
Files ? 109
Lines ? 10309
Branches ? 0
=========================================
Hits ? 9131
Misses ? 1009
Partials ? 169
Continue to review full report at Codecov.
|
/retest |
@@ -35,43 +38,143 @@ type Key struct { | |||
type Event struct { | |||
Key | |||
Type ChangeType | |||
|
|||
// Value refers the new value in the updated event. nil if the event type is delete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Value should be proto.Message, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/retest |
@jmuk PR needs rebase |
informer.AddEventHandler(s) | ||
go informer.Run(ctx.Done()) | ||
} | ||
} | ||
go s.closeAllWatches(ctx) | ||
time.Sleep(initializationPeriod) // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to see how much has be initialized ?
Make sure initialization period is not too small that we get a very small amount of config data.
pkg/config/crd/store.go
Outdated
dyn, err := dynamic.NewClient(s.conf) | ||
if err != nil { | ||
return err | ||
lwBuilder := s.listerWatcherBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
under what circumstances is s.listerWatcherBuilder
already set ?
This method is called only 1 time.
pkg/config/crd/store_test.go
Outdated
client := &Store{ | ||
conf: &rest.Config{}, | ||
discoveryBuilder: createFakeDiscovery, | ||
listerWatcherBuilder: lw, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah ok, so listWatcher is for test injection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
pkg/config/store/queue.go
Outdated
chout: make(chan store.Event), | ||
chin: make(chan store.Event), | ||
ctx: ctx, | ||
chout: make(chan Event), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the out chan
blocking chan? no length specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
return err | ||
} | ||
|
||
return convert(obj, spec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obj is map[string]interface{}
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
pkg/config/store/store2.go
Outdated
data := s.backend.List() | ||
result := make(map[Key]proto.Message, len(data)) | ||
for k, spec := range data { | ||
pbSpec, err := convertWithKind(spec, k.Kind, s.kinds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we have s.kinds
having convert
and converWithKind
methods is unnecessary.
you could access the map in the call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main point here is to create a clone of the message from s.kinds
, and then convert spec
into the clone (otherwise s.kinds
will be affected). Also, the same logic is used in queue.go
.
Maybe this structure wasn't clear. removed convertWithKind
, create cloneMessage
utility instead.
Over all the PR looks good, 2 things though.
|
Done. Also added tests. I think this improves the coverage (I'm afraid that Updated the PR description to describe the overview of this change. |
@sebastienvas code coverage numbers are not being updated. how do we fix it ? |
Per offline discussion, a new file Here's the local result of the code coverage:
|
/approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mandarjog The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
@mandarjog Which numbers ? the one from codecov or the one in pkg_check. cc @yutongz |
…f to `store` (istio#1120) Automatic merge from submit-queue Remove unnecessary methods from Store2, and moves handling of protobuf to `store` **Release note**: ```release-note none ``` This introduces `Store2Backend`, and keep `pkg/config/crd` untyped (spec is `map[string]interface{}`), and move the conversion part to `proto.Message` into `pkg/config/store`. With this, it is easier to add other types of store like FSstore, the same conversion logic can be reused. Also, we've agreed that the previous `Store2` interface had too many methods which wouldn't be used actually. This simplifies the interface, as removing `RegisterKind` (integrated into `Init`), removing kinds parameter from `Watch`, and removing of `Put` / `Delete`.
Release note:
This introduces
Store2Backend
, and keeppkg/config/crd
untyped (spec ismap[string]interface{}
), and move the conversion part toproto.Message
intopkg/config/store
. With this, it is easier to add other types of store like FSstore, the same conversion logic can be reused.Also, we've agreed that the previous
Store2
interface had too many methods which wouldn't be used actually. This simplifies the interface, as removingRegisterKind
(integrated intoInit
), removing kinds parameter fromWatch
, and removing ofPut
/Delete
.This change is