serialize dependencies into protobuf #165

changpingc · 2018-09-12T18:56:25Z

We'd like to ship dependencies out of thunder server process, which allows us to restore a subscription and avoid expensive initial computation when a client reconnects and resumes prior subscriptions. This also enables external dependency invalidator that can make thunder server stateless.

coveralls · 2018-09-12T19:14:45Z

Pull Request Test Coverage Report for Build 1064

84 of 152 (55.26%) changed or added relevant lines in 4 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.9%) to 65.5%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
reactive/util.go	0	1	0.0%
livesql/live.go	6	21	28.57%
reactive/rerunner.go	4	24	16.67%
livesql/marshal.go	74	106	69.81%

Files with Coverage Reduction	New Missed Lines	%
graphql/schemabuilder/reflect.go	4	71.56%

Totals
Change from base Build 1055:	0.9%
Covered Lines:	3444
Relevant Lines:	5258

💛 - Coveralls

berfarah · 2018-09-12T21:27:35Z

@changpingc Do you mind putting the reverts in a separate PR? 😬

changpingc · 2018-09-12T22:19:26Z

revert landed separately in #167.

berfarah · 2018-09-12T23:46:53Z

reactive/rerunner.go

+}
+
+type Serializable interface {
+	proto.Message


Could we make this a loose coupling by supporting an interface rather than importing proto? I'm open to being overruled here.

sure it can turn this into Marshal(), Unmarshal() probably

berfarah · 2018-09-13T23:28:17Z

sqlgen/reflect.go

+	case float64:
+		return float64(v)
+	case time.Time:
+		return time.Time(v)


There are some types here that we're doing nothing with. Do we need to handle them?

this handles all driver.Value.

This handles more than driver.Value - for example, uint32 is not a driver.Value, only int64 is

opened a separate PR #170

berfarah · 2018-09-13T23:28:35Z

livesql/marshal.go

+)
+
+func valueToField(value interface{}) (*thunderpb.Field, error) {
+	switch column := sqlgen.MakeCanonical(value).(type) {


What happens for custom types?

Talked offline. We are going to support it by storing driver.Value. See last commit!

berfarah · 2018-09-19T19:51:54Z

reactive/rerunner.go

+
+type Serializable interface {
+	// proto.Marshaler and proto.Unmarshaler.
+	Marshal() ([]byte, error)


Could use UnmarshalBinary/MarshalBinary and wrap our resource, but I'm cool with this / exposing this interface

I don't particularly like that.. Feels too clunky to wrap.

Yeah but would also be a built-in interface. I'm ok with not wrapping.

berfarah · 2018-09-19T19:54:43Z

reactive/rerunner.go

+
+	if serializable != nil {
+		depSet, ok := ctx.Value(dependencySetKey{}).(*dependencySet)
+		if ok && depSet != nil {


nit: seems nice to have getter/setter methods on the dependency set rather than handling it here and below

berfarah · 2018-09-19T19:55:56Z

reactive/util.go

 )

 func InvalidateAfter(ctx context.Context, d time.Duration) {
 	r := NewResource()
 	timer := time.AfterFunc(d, r.Invalidate)
 	r.Cleanup(func() { timer.Stop() })
-	AddDependency(ctx, r, nil)
+	AddDependency(ctx, r, &thunderpb.ExpirationTime{Time: time.Now().Add(d)})


thought - if we have this here, are we still coupling ourselves to proto?

(maybe fine?)

yes we are.. I feel that's fine.

berfarah · 2018-09-19T19:56:49Z

livesql/live.go

@@ -75,7 +76,7 @@ func (t *dbTracker) processBinlog(update *update) {
 	}
 }

-func (t *dbTracker) registerDependency(ctx context.Context, table string, tester sqlgen.Tester) {
+func (t *dbTracker) registerDependency(ctx context.Context, table string, tester sqlgen.Tester, filter sqlgen.Filter) error {


If we have the filter, do we need the tester to be passed in?

To build a tester, we need schema object. This change seems like the smallest change I can make

berfarah · 2018-09-19T21:13:57Z

sqlgen/reflect.go

+	case float64:
+		return float64(v)
+	case time.Time:
+		return time.Time(v)


This handles more than driver.Value - for example, uint32 is not a driver.Value, only int64 is

…ncies There are 2 kinds of dependencies in thunder we want to serialize: reactive.InvalidateAt and livesql.Query. This commit creates protobuf definitions for each type and their gogofast generated files. The serialized dependencies are useful because we want to ship them out of process to an external service.

This commit adds one required nil-able argument that is the serializable form of the dependency. This argument is collected in a dependency set which is accessable via Dependencies(ctx) call.

This commit adds conversion between thunderpb.SQLFilter and sqlgen.Tester. It assumes filter values are driver.Value type for now, and we will relax that condition later.

changpingc · 2018-09-20T18:52:38Z

@berfarah I think this is ready to land! It should not introduce any additional error in existing API.

berfarah · 2018-09-20T19:10:23Z

@changpingc given the amount of changes here, do you think a second pair of eyes might be useful?

berfarah

Approving - though might be good to get some additional eyes from @stephen. The diff count on this PR is deceiving.

berfarah · 2018-09-20T22:06:16Z

livesql/marshal_test.go

+		{
+			name: "id int64 ptr to int64",
+			filter: sqlgen.Filter{
+				"id": &one,


nil test case?

zdylag

👍 Like the tests

willhug · 2018-10-02T15:00:49Z

thunderpb/dependency.proto

+  Time = 8;
+}
+
+message Field {


ahh, think I found what's causing a lot of cloudserver OOMs, I think this field is quite large and taking up much more memory than usual whenever we create a dependency.

Have we thought of using the proto oneof annotation here? Not sure if it actually reduces the allocation in code, just a thought.

here's one of our heap profiles: https://s3.internal.samsara.com/s3/samsara-prod-profile/2018-10-02/cloudserver/1438-prod-23996c8a-f0f5-47be-960a-d96d639a2f26-2wHXqesa-heap.pprof.svg

changpingc requested a review from berfarah September 12, 2018 18:56

changpingc changed the title ~~serialize dependencies~~ serialize dependencies into protobuf Sep 12, 2018

changpingc force-pushed the changping/serialize branch from 7547ade to 867938c Compare September 12, 2018 19:09

changpingc force-pushed the changping/serialize branch from 867938c to e04ebde Compare September 12, 2018 20:18

changpingc force-pushed the changping/serialize branch from e04ebde to ec2b8f9 Compare September 12, 2018 22:18

changpingc force-pushed the changping/serialize branch 6 times, most recently from bb9fa86 to a7bee55 Compare September 13, 2018 20:50

berfarah force-pushed the master branch from 3341c4c to 0fb7086 Compare September 13, 2018 21:15

berfarah reviewed Sep 13, 2018

View reviewed changes

changpingc force-pushed the changping/serialize branch 7 times, most recently from 810bd16 to 315c964 Compare September 14, 2018 21:16

berfarah reviewed Sep 19, 2018

View reviewed changes

changpingc force-pushed the changping/serialize branch 4 times, most recently from 97c986c to e968849 Compare September 20, 2018 18:15

changpingc changed the base branch from master to changping/value-filter September 20, 2018 18:15

changpingc force-pushed the changping/serialize branch from e968849 to 458de2e Compare September 20, 2018 18:23

changpingc added 6 commits September 20, 2018 11:50

go vendor: add gogoproto

44989e2

go reactive: AddDependency takes an addtional serializable

6b90351

This commit adds one required nil-able argument that is the serializable form of the dependency. This argument is collected in a dependency set which is accessable via Dependencies(ctx) call.

go thunder/reactive: InvalidateAt/After stores expiration time

bf9242f

go thunder/livesql: serialize and restore sqlgen.Tester

4035eaf

This commit adds conversion between thunderpb.SQLFilter and sqlgen.Tester. It assumes filter values are driver.Value type for now, and we will relax that condition later.

go sqlgen: test batched query matches rows against filter

3c69e0b

changpingc changed the base branch from changping/value-filter to master September 20, 2018 18:50

changpingc force-pushed the changping/serialize branch from 458de2e to 3c69e0b Compare September 20, 2018 18:50

changpingc requested review from jellevandenhooff and stephen September 20, 2018 19:14

berfarah approved these changes Sep 20, 2018

View reviewed changes

berfarah reviewed Sep 20, 2018

View reviewed changes

livesql/marshal_test.go Outdated

{

name: "id int64 ptr to int64",

filter: sqlgen.Filter{

"id": &one,

Copy link

Contributor

berfarah Sep 20, 2018

Choose a reason for hiding this comment

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

nil test case?

changpingc force-pushed the changping/serialize branch from b86d5cf to 49f7dff Compare September 20, 2018 22:18

go livesql: marshal non driver.Value type filters

e7baaf5

changpingc force-pushed the changping/serialize branch from 49f7dff to e7baaf5 Compare September 20, 2018 22:19

changpingc requested a review from zdylag September 20, 2018 22:22

zdylag approved these changes Sep 20, 2018

View reviewed changes

changpingc merged commit 3ff10eb into master Sep 20, 2018

changpingc deleted the changping/serialize branch September 20, 2018 22:32

changpingc restored the changping/serialize branch September 20, 2018 22:33

changpingc deleted the changping/serialize branch September 20, 2018 22:34

willhug reviewed Oct 2, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

serialize dependencies into protobuf #165

serialize dependencies into protobuf #165

changpingc commented Sep 12, 2018

coveralls commented Sep 12, 2018 •

edited

Loading

berfarah commented Sep 12, 2018

changpingc commented Sep 12, 2018

berfarah Sep 12, 2018

changpingc Sep 14, 2018

changpingc Sep 14, 2018

berfarah Sep 13, 2018

changpingc Sep 14, 2018

berfarah Sep 19, 2018

changpingc Sep 20, 2018

berfarah Sep 13, 2018

changpingc Sep 14, 2018

berfarah Sep 19, 2018

changpingc Sep 19, 2018 •

edited

Loading

berfarah Sep 20, 2018

berfarah Sep 19, 2018

changpingc Sep 19, 2018

berfarah Sep 19, 2018

berfarah Sep 19, 2018

changpingc Sep 19, 2018

berfarah Sep 19, 2018

changpingc Sep 19, 2018

berfarah Sep 19, 2018

changpingc commented Sep 20, 2018

berfarah commented Sep 20, 2018

berfarah left a comment

berfarah Sep 20, 2018

zdylag left a comment

willhug Oct 2, 2018

willhug Oct 2, 2018

serialize dependencies into protobuf #165

serialize dependencies into protobuf #165

Conversation

changpingc commented Sep 12, 2018

coveralls commented Sep 12, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1064

💛 - Coveralls

berfarah commented Sep 12, 2018

changpingc commented Sep 12, 2018

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

changpingc Sep 19, 2018 • 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

changpingc commented Sep 20, 2018

berfarah commented Sep 20, 2018

berfarah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdylag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 12, 2018 •

edited

Loading

changpingc Sep 19, 2018 •

edited

Loading