Generate data store code #2039

aaronlehmann · 2017-03-15T02:50:16Z

It has become difficult to add top-level object types. Doing so involves copying/pasting a lot of code in the store, and potentially making mistakes. We had considered changing the object model to work around this, but decided that we already have the right object model - we just have a code problem. Code generation can solve the code duplication and tedium of adding top-level objects.

This PR is a start at doing that. First, it simplifies the object-specific code in the store by generating interface methods on each top-level type, so the store doesn't have to use a wrapper type to provide methods like ID(), Meta(), etc.

As part of this, the event types had to be moved into the api package to avoid a circular dependency. While we're at it, we code generate those event types, which were another annoying bit of boilerplate.

Finally, this can generate a few basic indexers. More will come, perhaps by marking the fields of interest in the .proto file with an extension option.

cc @docker/core-swarmkit-maintainers

codecov · 2017-03-15T02:59:17Z

Codecov Report

Merging #2039 into master will increase coverage by 0.47%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #2039      +/-   ##
==========================================
+ Coverage   53.37%   53.85%   +0.47%     
==========================================
  Files         111      111              
  Lines       19703    19211     -492     
==========================================
- Hits        10517    10346     -171     
+ Misses       7917     7605     -312     
+ Partials     1269     1260       -9

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 299916c...48b7518. Read the comment docs.

aaronlehmann · 2017-03-15T18:40:21Z

Updated to also generate code for NewStoreAction.

aaronlehmann · 2017-03-17T17:51:59Z

Rebased. Let's try to get this in. I think it's a big improvement. We can continue to make improvements as we go.

dongluochen · 2017-03-20T22:55:03Z

Can you add a test for code generation to validate the generated code is expected?

aaronlehmann · 2017-03-20T22:58:35Z

@dongluochen: What kind of test do you have in mind?

There are unit tests in manager/state/store that test the store functionality. We could theoretically code-generate these for each object type so that they are more exhaustive. That's a bit tricky though - I don't think we can do that within the current code generation framework.

dongluochen · 2017-03-20T23:00:13Z

There are unit tests in manager/state/store that test the store functionality.

If they have covered the generated functions, that would be enough.

aaronlehmann · 2017-03-20T23:00:48Z

Understood. Let me give it a try.

cyli · 2017-03-20T23:44:21Z

protobuf/plugin/storeobject/storeobject.go

+
+	d.P("func (indexer ", ccTypeName, "IndexerByName) FromObject(obj interface{}) (bool, []byte, error) {")
+	d.In()
+	d.P("m := obj.(*", ccTypeName, ")")


In the code that the auto-generation replaces, we check if the cast succeeds, else we panic saying that the object passed to FromObject is invalid. Do we also need to do that here (and also in the CustomIndexer and IndexByID in the auto-generated code?

Good catch but I left it out because this would only happen if the code setting up the indices was wrong, and if it was, this will panic anyway the first time one of those objects is added to the store. Having a fancier custom panic for this use case didn't seem worth generating the extra code.

Ah ok, then do we want to remove the panic check in the other indexers then? (e.g. the other Task ones)

Removing them should be fine, but I don't think it matters either way.

Sounds good - I have no strong feelings, was just wondering if we should be consistent.

cyli · 2017-03-21T00:12:17Z

api/objects.proto

@@ -25,6 +26,8 @@ message Meta {

 // Node provides the internal node state as seen by the cluster.
 message Node {
+	option (docker.protobuf.plugin.store_object) = { table: "node" };


Non-blocking dumb question: I am probably just not seeing it, but where is this option used in the code generation?

It is not used yet. I anticipated automatically generating the index schema, but it's not there yet. On further consideration, if/when this happens it should just be generated from a lowercased version of the object name. I'll remove this.

Note that #2034 extends this protobuf option with further fields.

Oh cool, ok, thanks!

aaronlehmann · 2017-03-21T01:02:37Z

@dongluochen: @cyli generously generated cross-package coverage info, and found that the unit tests are covering most of the generated code. Not every single generated function is covered (for example, particular events that are not used by the unit tests), but since a lot of these generated functions are essentially duplicates of each other, it looks like the existing tests are giving us a very good cross section that covers essentially everything that the code generation is doing. I can give you the coverage profile if you're interested.

aaronlehmann · 2017-03-21T01:20:32Z

Rebased, and removed table field.

dongluochen · 2017-03-21T01:45:47Z

LGTM

cyli · 2017-03-21T01:48:03Z

LGTM

aluzzardi · 2017-03-21T01:54:03Z

manager/state/store/clusters.go

 }

 // UpdateCluster updates an existing cluster in the store.
 // Returns ErrNotExist if the cluster doesn't exist.
 func UpdateCluster(tx Tx, c *api.Cluster) error {
 	// Ensure the name is either not in use or already used by this same Cluster.
 	if existing := tx.lookup(tableCluster, indexName, strings.ToLower(c.Spec.Annotations.Name)); existing != nil {
-		if existing.ID() != c.ID {
+		if existing.GetID() != c.ID {


nit: Why the change? Typically in Go getters are not prefixed by Get

ID() would conflict with the ID field.

aluzzardi · 2017-03-21T01:58:19Z

LGTM

Couple of questions:

Would it be possible to generate our stuff under different packages? I guess not but worth asking. For instance, api.ClusterIndexerByName etc could live in api/indexers
Any way the code generator could use go templates or is it impossible/overkill? It's really hard to watch

aaronlehmann · 2017-03-21T02:00:11Z

Would it be possible to generate our stuff under different packages? I guess not but worth asking. For instance, api.ClusterIndexerByName etc could live in api/indexers

I would like this but I don't know of a way to do it. The code generation that's part of protobuf seems to just add stuff to the .pb.go file.

Any way the code generator could use go templates or is it impossible/overkill? It's really hard to watch

Seems tricky. Anyway, it would definitely be something for a followup.

Generate interface methods on top-level objects so they can be used directly by the store instead of needing a wrapper. Generate some basic indexers. Signed-off-by: Aaron Lehmann <[email protected]>

Signed-off-by: Aaron Lehmann <[email protected]>

aaronlehmann force-pushed the codegen-store branch from d040c4d to 6c72dfd Compare March 17, 2017 17:51

aaronlehmann changed the title ~~[WIP] Generate data store code~~ Generate data store code Mar 17, 2017

aaronlehmann mentioned this pull request Mar 18, 2017

External watch API #2034

Merged

cyli reviewed Mar 20, 2017

View reviewed changes

cyli reviewed Mar 21, 2017

View reviewed changes

aaronlehmann force-pushed the codegen-store branch from 6c72dfd to 284e647 Compare March 21, 2017 01:20

aluzzardi reviewed Mar 21, 2017

View reviewed changes

aaronlehmann added 3 commits March 20, 2017 19:01

Generate code for store implementation

1aa34db

Generate interface methods on top-level objects so they can be used directly by the store instead of needing a wrapper. Generate some basic indexers. Signed-off-by: Aaron Lehmann <[email protected]>

Generate code for NewStoreAction

6622457

Signed-off-by: Aaron Lehmann <[email protected]>

Remove Name field from Object

48b7518

Signed-off-by: Aaron Lehmann <[email protected]>

aaronlehmann force-pushed the codegen-store branch from 284e647 to 48b7518 Compare March 21, 2017 02:01

aaronlehmann merged commit 61c4d67 into moby:master Mar 21, 2017

aaronlehmann deleted the codegen-store branch March 21, 2017 02:11

aaronlehmann mentioned this pull request Mar 22, 2017

Generic Task Runtime support #2014

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate data store code #2039

Generate data store code #2039

aaronlehmann commented Mar 15, 2017 •

edited

Loading

codecov bot commented Mar 15, 2017 •

edited

Loading

aaronlehmann commented Mar 15, 2017

aaronlehmann commented Mar 17, 2017

dongluochen commented Mar 20, 2017

aaronlehmann commented Mar 20, 2017

dongluochen commented Mar 20, 2017

aaronlehmann commented Mar 20, 2017

cyli Mar 20, 2017

aaronlehmann Mar 21, 2017

cyli Mar 21, 2017

aaronlehmann Mar 21, 2017

cyli Mar 21, 2017

cyli Mar 21, 2017

aaronlehmann Mar 21, 2017

cyli Mar 21, 2017

aaronlehmann commented Mar 21, 2017

aaronlehmann commented Mar 21, 2017

dongluochen commented Mar 21, 2017

cyli commented Mar 21, 2017

aluzzardi Mar 21, 2017

aaronlehmann Mar 21, 2017

aluzzardi commented Mar 21, 2017

aaronlehmann commented Mar 21, 2017

Generate data store code #2039

Generate data store code #2039

Conversation

aaronlehmann commented Mar 15, 2017 • edited Loading

codecov bot commented Mar 15, 2017 • edited Loading

Codecov Report

aaronlehmann commented Mar 15, 2017

aaronlehmann commented Mar 17, 2017

dongluochen commented Mar 20, 2017

aaronlehmann commented Mar 20, 2017

dongluochen commented Mar 20, 2017

aaronlehmann commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Mar 21, 2017

aaronlehmann commented Mar 21, 2017

dongluochen commented Mar 21, 2017

cyli commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aluzzardi commented Mar 21, 2017

aaronlehmann commented Mar 21, 2017

aaronlehmann commented Mar 15, 2017 •

edited

Loading

codecov bot commented Mar 15, 2017 •

edited

Loading