Skip to content
This repository has been archived by the owner on Feb 9, 2022. It is now read-only.

Remove unnecessary methods from Store2, and moves handling of protobuf to store #1120

Merged
merged 22 commits into from
Aug 24, 2017

Conversation

jmuk
Copy link
Contributor

@jmuk jmuk commented Aug 22, 2017

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.


This change is Reviewable

func (q *eventQueue) run() {
loop:
for {
select {
case <-q.ctx.Done():
fmt.Printf("done0\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

fmt? use glog.Info ?

Copy link
Contributor Author

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.

// 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 {
Copy link
Contributor

@mandarjog mandarjog Aug 22, 2017

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.


func convert(spec map[string]interface{}, target proto.Message) error {
// This is inefficient; convert to a protobuf message through JSON.
// TODO: use reflect.
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

backend Store2Backend
}

// NewStore2 creates a new Store2 instance with the specified backend.
Copy link
Contributor

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) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@codecov
Copy link

codecov bot commented Aug 22, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@8dd7459). Click here to learn what that means.
The diff coverage is 76.38%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1120   +/-   ##
=========================================
  Coverage          ?   88.57%           
=========================================
  Files             ?      109           
  Lines             ?    10309           
  Branches          ?        0           
=========================================
  Hits              ?     9131           
  Misses            ?     1009           
  Partials          ?      169
Impacted Files Coverage Δ
pkg/config/crd/init.go 0% <0%> (ø)
pkg/config/inventory.go 0% <0%> (ø)
pkg/config/crd/resource.go 100% <100%> (ø)
pkg/config/store/convert.go 100% <100%> (ø)
pkg/config/store/store2.go 100% <100%> (ø)
pkg/config/crd/store.go 82.11% <85.71%> (ø)
pkg/config/store/queue.go 93.02% <93.02%> (ø)

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 8dd7459...f029fce. Read the comment docs.

@jmuk
Copy link
Contributor Author

jmuk commented Aug 22, 2017

/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.
Copy link
Contributor

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jmuk
Copy link
Contributor Author

jmuk commented Aug 22, 2017

/retest

@istio-merge-robot
Copy link

@jmuk PR needs rebase

informer.AddEventHandler(s)
go informer.Run(ctx.Done())
}
}
go s.closeAllWatches(ctx)
time.Sleep(initializationPeriod) // TODO
Copy link
Contributor

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.

dyn, err := dynamic.NewClient(s.conf)
if err != nil {
return err
lwBuilder := s.listerWatcherBuilder
Copy link
Contributor

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.

client := &Store{
conf: &rest.Config{},
discoveryBuilder: createFakeDiscovery,
listerWatcherBuilder: lw,
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes.

chout: make(chan store.Event),
chin: make(chan store.Event),
ctx: ctx,
chout: make(chan Event),
Copy link
Contributor

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

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)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@mandarjog
Copy link
Contributor

Over all the PR looks good, 2 things though.

  1. Please add one paragraph of design notes
  2. Please increase the code coverage? store2.go, store.go and resource.go have especially low code coverage.

@jmuk
Copy link
Contributor Author

jmuk commented Aug 23, 2017

Done. Also added tests. I think this improves the coverage (I'm afraid that store2.go's coverage won't be very high, since it still has some code path used to connect with real k8s servers).

Updated the PR description to describe the overview of this change.

@mandarjog mandarjog self-assigned this Aug 23, 2017
@mandarjog
Copy link
Contributor

@sebastienvas code coverage numbers are not being updated. how do we fix it ?

@jmuk
Copy link
Contributor Author

jmuk commented Aug 23, 2017

Per offline discussion, a new file init.go was created in pkg/config/crd to scope the code that requires actual kubernetes services for initialization.

Here's the local result of the code coverage:

pkg/config/crd/init.go: 0%
pkg/config/crd/resource.go: 100%
pkg/config/crd/store.go: 91.4%
pkg/config/store/convert.go: 100%
pkg/config/store/queue.go: 92.6%
pkgconfig/store/store2.go: 100%

@mandarjog
Copy link
Contributor

/approve

@mandarjog
Copy link
Contributor

/lgtm

@istio-merge-robot
Copy link

[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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@istio-merge-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link

Automatic merge from submit-queue

@istio-merge-robot istio-merge-robot merged commit b5aead1 into istio:master Aug 24, 2017
@sebastienvas
Copy link
Contributor

@mandarjog Which numbers ? the one from codecov or the one in pkg_check. cc @yutongz

rvkubiak pushed a commit to rvkubiak/mixer that referenced this pull request Oct 10, 2017
…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`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants