Use Gengo to generate DeepCopy methods #54

adutra · 2022-05-28T10:27:57Z

Fixes #31.

adutra · 2022-05-28T10:33:35Z

Notes for Reviewers

1. Overview

The solution proposed here is based on a tool named Gengo. Gengo is a code generator framework widely used in Kubernetes. It works like a charm for generating DeepCopy methods that work exactly as we need them to. The main painpoint of my work was just to figure out how to use the tool in standalone mode (outisde of Kubernetes).

The tool work with annotations placed on the types we want to generate DeepCopy methods for.

To generate a simple DeepCopy method:

// +k8s:deepcopy-gen=true
type Inet struct {
	Addr net.IP
	Port int32
}

This will trigger the generation of a file named deepcopy_generated.go in the package directory.

When the DeepCopy method is part of an interface, we need to first declare a method in the interface called DeepCopyX where X is the name of the return type. E.g.:

type Message interface {
	DeepCopyMessage() Message
}

Then we can annotate each implementing type with:

// +k8s:deepcopy-gen=true
// +k8s:deepcopy-gen:interfaces=github.com/datastax/go-cassandra-native-protocol/message.Message
type Startup struct {
	Options map[string]string
}

Note: while very flexible, the tool does NOT allow to change the tag name or the generated method names. (We would need to create our own tool based on the Gengo engine).

adutra · 2022-05-28T11:57:52Z

2. Running the tool

This PR introduces a Makefile to facilitate running the tool.

The Makefile has a deep-copy target that:

Clones the Gengo repository;
Runs go install from the cloned repository and installs the binary in ./build/bin/deepcopy-gen;
Runs ./build/bin/deepcopy-gen to generate methods in the following packages: primitive, message, datatype, frame and segment.

3. Tests

The tests for Clone methods could now be removed but instead I chose to keep them as a proof that the new DeepCopy methods are behaving as expected.

adutra · 2022-05-28T11:59:43Z

primitive/inet.go

@@ -23,6 +23,8 @@ import (

 // [inet] (net.IP + port)

+// Inet is the [inet] protocol type. It is the combination of a net.IP + port number.
+// +k8s:deepcopy-gen=true
 type Inet struct {


This type wasn't being cloned before, but it needs deep copy.

adutra · 2022-05-28T12:00:01Z

primitive/nillables.go

@@ -14,6 +14,7 @@

 package primitive

+// +k8s:deepcopy-gen=true
 type NillableInt32 struct {


FYI this will go away with #52.

adutra · 2022-05-28T12:00:32Z

primitive/reasonmap.go

@@ -26,6 +26,9 @@ import (
 // Note: a reasonmap is inherently a map, but it is not modeled as a map in Go because [inetaddr]
 // is not a valid map key type.

+// FailureReason is a map entry for a <reasonmap>; it contains the endpoint that failed and the corresponding failure
+// code.
+// +k8s:deepcopy-gen=true
 type FailureReason struct {


Again this type wasn't being cloned.

adutra · 2022-05-28T12:01:09Z

primitive/uuid.go

@@ -26,11 +26,10 @@ const LengthOfUuid = 16

 type UUID [16]byte

-func (u *UUID) Clone() *UUID {
+func (u *UUID) DeepCopy() *UUID {


This is the only DeepCopy method that had to be kept manual because the generator does not handle array types.

adutra · 2022-05-28T12:04:18Z

Makefile

@@ -0,0 +1,144 @@
+# From https://raw.githubusercontent.com/vincentbernat/hellogopher/1cf9ebeeaea41d2e159e196f7d176d6b12ff0eb2/Makefile


This Makefile is inspired by a popular template for Go projects. I didn't invent all of this. The only modifications I brought are:

Modification of the all target since this project does not have a main package.

Changed default output dir for all targets to ./build.

Installation of additional tools:

Gengo

Addlicense

Mockery

New targets:

deep-copy

mocks

check-license and update-license

pseudo-version

adutra · 2022-05-28T12:06:47Z

Makefile

+export GO111MODULE=on
+
+.PHONY: all
+all: check-license mocks deep-copy fmt lint test-coverage | $(BIN)


Note: running the all target right now will bring up hundreds of lint problems, most of them related to exported types without comments. I did not fix those in this PR. I plan to do it later on.

adutra · 2022-05-28T12:09:36Z

Makefile

+.PHONY: all
+all: check-license mocks deep-copy fmt lint test-coverage | $(BIN)
+
+# Tools


The tool targets all work on the same principle: use go install to install build tools under bin. I added 3 tools: addlicense, mockery and deeepcopy-gen.

A few notes:

deepcopy-gen needs a different installation procedure because it contains replace directives.

mockery is being installed correctly so far but the author does not recommend installing this way. If we ever have problems in the future we'll need to install mockery differently but so far it's working.

adutra · 2022-05-28T12:09:57Z

Makefile

+# See https://stackoverflow.com/questions/52242077/go-modules-finding-out-right-pseudo-version-vx-y-z-timestamp-commit-of-re
+PSEUDO_VERSION ?= $(shell TZ=UTC git --no-pager show --quiet --abbrev=12 --date='format-local:%Y%m%d%H%M%S' --format="%cd-%h" 2> /dev/null || echo v0)
+.PHONY: pseudo-version
+pseudo-version:


Useful target to print the Go pseudo-version of a given commit.

adutra · 2022-05-28T12:11:58Z

message/batch.go

-	// the statement to execute.
-	QueryOrId interface{}
+	// The CQL statement to execute. Exactly one of Query or Id must be present, never both.
+	Query string


This change was induced by the generator because it does not support interface{} types. But I think this is a good change anyways, I don't even know why I used a single field for both Query and Id while it's so much simpler to use 2 distinct fields.

adutra · 2022-05-28T12:19:27Z

segment/segment.go

@@ -24,12 +24,14 @@ import (
 // larger frame spanning multiple segments (multi-segment part). It can also be optionally compressed with Lz4.
 // Segments were introduced in protocol v5 to improve compression efficiency (especially for small messages) and to
 // introduce error detection in the native protocol.
+// +k8s:deepcopy-gen=true
 type Segment struct {


Types in this package did not have Clone methods so far, but for the sake of completeness, I'm generating methods for them as well.

adutra · 2022-05-28T12:36:10Z

datatype/custom.go

+// CustomType is a data type that represents a CQL custom type.
+// +k8s:deepcopy-gen=true
+// +k8s:deepcopy-gen:interfaces=github.com/datastax/go-cassandra-native-protocol/datatype.DataType
+type CustomType struct {


In this package, some interfaces are now structs. This is because the tool cannot generate deepcopy methods for unexported structs.

But in fact, I don't even know why I created these interfaces in the first place. The only impl is the associated struct, so they seem pretty overkill.

Also note: the names are going to change with #53.

adutra · 2022-05-28T12:41:25Z

Makefile

+test-bench:   ARGS=-run=__absolutelynothing__ -bench=. ## Run benchmarks
+test-short:   ARGS=-short        ## Run only short tests
+test-verbose: ARGS=-v            ## Run tests in verbose mode with coverage reporting
+test-race:    ARGS=-race         ## Run tests with race detector


Interesting to note: test-race reports about a dozen race conditions in the client package.

Yeah it's not surprising as I already ran into a couple of them in #36 and #38 . We're only using the package for integration tests so we're not too worried but we might need to take a look at these issues at some point.

adutra · 2022-05-28T14:55:05Z

go.mod

@@ -1,12 +1,19 @@
 module github.com/datastax/go-cassandra-native-protocol

-go 1.16
+go 1.17


I'm changing to 1.17 just because the Makefile requires 1.17 (to use go install). Note that this has little to no impact on users of this library as it remains backwards-compatible as long as we don't use any language feature specific to 1.17 (which we don't).

joao-r-reis

Looks great! Keeping the Clone() tests was a great decision 👍

adutra commented May 28, 2022

View reviewed changes

adutra force-pushed the deep-copy branch 3 times, most recently from 729c316 to fcb42bc Compare May 28, 2022 14:51

adutra commented May 28, 2022

View reviewed changes

Use Gengo to generate DeepCopy methods

86d39b8

adutra force-pushed the deep-copy branch from fcb42bc to 86d39b8 Compare May 28, 2022 15:03

joao-r-reis approved these changes Jul 5, 2022

View reviewed changes

adutra merged commit e92ad89 into datastax:main Jul 6, 2022

adutra deleted the deep-copy branch July 6, 2022 09:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Gengo to generate DeepCopy methods #54

Use Gengo to generate DeepCopy methods #54

adutra commented May 28, 2022

adutra commented May 28, 2022

adutra commented May 28, 2022 •

edited

Loading

adutra May 28, 2022

adutra May 28, 2022

adutra May 28, 2022

adutra May 28, 2022

adutra May 28, 2022 •

edited

Loading

adutra May 28, 2022

adutra May 28, 2022

adutra May 28, 2022

adutra May 28, 2022

joao-r-reis Jul 5, 2022

adutra May 28, 2022

adutra May 28, 2022

adutra May 28, 2022

adutra May 28, 2022

joao-r-reis Jul 5, 2022

adutra May 28, 2022

joao-r-reis left a comment

		@@ -0,0 +1,144 @@
		# From https://raw.githubusercontent.com/vincentbernat/hellogopher/1cf9ebeeaea41d2e159e196f7d176d6b12ff0eb2/Makefile

Use Gengo to generate DeepCopy methods #54

Use Gengo to generate DeepCopy methods #54

Conversation

adutra commented May 28, 2022

adutra commented May 28, 2022

Notes for Reviewers

1. Overview

adutra commented May 28, 2022 • edited Loading

2. Running the tool

3. Tests

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

adutra May 28, 2022 • edited Loading

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

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

joao-r-reis left a comment

Choose a reason for hiding this comment

adutra commented May 28, 2022 •

edited

Loading

adutra May 28, 2022 •

edited

Loading