Register dispatcher in the default dig graph #304

ghost · 2017-02-21T02:27:57Z

Use DIG to store the YAPRC dispatcher.

Right now people are confused about rpc.Dispatcher(), which returns you a dispatcher only if service is started and testing handlers becomes much harder. Proposed solution is to inject it in the default dig graph.

mention-bot · 2017-02-21T02:27:58Z

@alsamylkin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Lupie, @sectioneight and @shawnburke to be potential reviewers.

glibsm · 2017-02-21T02:30:22Z

doc.go

 //
+// • Logging backed by the zap logger


Looks like file diff shows the wrong symbol, but the conversation view is fine :/

coveralls · 2017-02-21T02:30:48Z

Coverage decreased (-0.3%) to 91.748% when pulling a402a70 on diDispatcher into 175cddf on master.

glibsm · 2017-02-21T02:33:07Z

modules/rpc/yarpc.go

@@ -49,6 +50,7 @@ type YARPCModule struct {
 	log       ulog.Log
 	stateMu   sync.RWMutex
 	isRunning bool
+	di        dig.Graph


I'm sure you had your reasons, but I don't know if it's the right thing to attach di object to the YARPC module. Can we think of alternative approaches? Is this purely to make testing easier?

Wouldn't it make testing the handlers difficult, as you'd have to either:

a. Rely on the assumption that we fall back onto the default graph
b. Pass around the reference to the module

Why not just use one global graph for UberFx? Makes things much simpler.

Short answer because of snowflakes and tests: it is much easier to write tests with custom graphs, especially when/if yarpc team will add an in memory transport.

Also, I was able to remove di from the struct. Wrote a long rant, why it is hard and then hit an eureka moment..

madhuravi · 2017-02-21T05:45:48Z

dig/doc.go

@@ -20,7 +20,7 @@

 // Package dig is the Dependency Injection Graph.
 //
-// Package dig provides a fairly opinionated way of resolving object dependencies.
+// package dig provides an opinionated way of resolving object dependencies.


Rant alert :P I think we overuse the word opinionated, I don't see dig as opinionated particularly. Each type of DI provides a different way of resolving dependencies that's all.

Why not "Dig resolves dependencies and provides users with an instantiated object when requested"

This was picked by me randomly, here is the Glib's fix:
https://github.com/uber-go/fx/pull/305/files

madhuravi · 2017-02-21T05:45:56Z

dig/doc.go

@@ -20,7 +20,7 @@



Missing README files?

Ah, my bad. We have been forgetting doc.go in commits :)

madhuravi · 2017-02-21T05:47:01Z

doc.go

@@ -27,19 +27,28 @@
 //
 // Status
 //
-// Alpha. Expect minor API changes and bug fixes. (Beta release coming soon)
+// Beta. Expect minor API changes and bug fixes. See our changelog (CHANGELOG.md)for more.


This should not show up as a change, pull latest changes.

madhuravi · 2017-02-21T05:47:28Z

doc.go

 //
+// • Logging backed by the zap logger


Likewise, there are no changes to this file from this PR. It should go away when you pull changes.

dmcaulay · 2017-02-21T06:42:29Z

modules/rpc/yarpc_options.go

+	}
+
+	// Intentionally panic if someone adds non-graph to Items.
+	return g.(dig.Graph)


Should you call panic explicitly here? It seems kinda random to force a type assertion panic.

Hmm, how an explicit panic will be better than a regular one? It will provide all the types needed:
panic: interface conversion: interface is *dig.graph, not string

I misread this earlier. Ignore the previous comment.

coveralls · 2017-02-21T08:24:10Z

Changes Unknown when pulling a3d1048 on diDispatcher into ** on master**.

coveralls · 2017-02-21T08:27:04Z

Changes Unknown when pulling 8bc5fb6 on diDispatcher into ** on master**.

coveralls · 2017-02-21T08:29:01Z

Changes Unknown when pulling 8bc5fb6 on diDispatcher into ** on master**.

coveralls · 2017-02-21T08:42:30Z

Changes Unknown when pulling 00f640e on diDispatcher into ** on master**.

anuptalwalkar · 2017-02-21T16:54:33Z

modules/rpc/yarpc_options.go

@@ -71,3 +73,20 @@ func onewayInboundMiddlewareFromCreateInfo(mci service.ModuleCreateInfo) []middl
 	// Intentionally panic if programmer adds non-middleware slice to the data
 	return items.([]middleware.OnewayInbound)
 }
+
+func withGraph(graph dig.Graph) modules.Option {


wait a minute. Why is the graph passed as an object? I thought the whole point of having DI is to perform dependency injection and not pass around as a dependency baggage. These objects should just be access d as 'dig.Resolve()'. Otherwise Im having hard time understanding the use of graph vs passing a struct or an interface map.

This.

We need to be really careful about how users are meant to use this.

If we set users up so that they inject the graph into their objects, then we've messed up - the graph isn't supposed to leave main.

@glibsm
Totally agree and we shouldn't use it as an object in fx as well. I believe this is a situation where global graph is required and should be restricted to be used only within DIG. That way we can have sane dig.Resolve, dig.Register APIs and restrict baggage passing use cases.

I've added some thoughts to the dig issue on idiomatic usage. #285

I think it might be better to start out with more constrained usage patterns until we get more familiar with dig.

This is a private option to a constructor for testing.

What you are proposing @anup is to have a graph to be a global dump of all objects, so we essentially left with the same globals as before(actually worse: we added an extra dependency). This is worse for testing, hard to have multiple objects of the same type, etc

I don't understand how it is worst for testing. It might be easier to test here by passing graph, but then how do you test DI outside fx? There may be more work involved in setting up DI for test, but it will at least be transparent.
The graph is a dump of object in either case, but right now we are exposing it to test passing it around. IMHO dig users should not be aware of the graph.

Because you can't run tests in parallel, global graph...
Not only this, but we can have multiple dispatchers if we really want and "know what we are doing" :)

I am confused about "test DI outside fx", why do we care in this module about it? We take it as an interface..

Dig users should be aware of graph, otherwise it can be misused.

Synced offline and decided to use a global dig graph and add ability to run tests in parallel through dig, not a local field.

coveralls · 2017-02-21T20:46:12Z

Changes Unknown when pulling 6270af9 on diDispatcher into ** on master**.

HelloGrayson · 2017-02-21T22:05:58Z

modules/rpc/thrift_test.go

@@ -74,6 +76,11 @@ modules:

 	testInitRunModule(t, goofy[0], mci)
 	testInitRunModule(t, gopher[0], mci)
+
+	// Dispatcher must be resolved in the default graph


Why does FX need a default graph at all? Why not just instantiate 1 graph when FX is instantiated?

Well, it is about what graph services are going to use to resolve a dispatcher.
From the thread above we decided to use a default graph and not pass it as a parameter, until we find a better way to do it with dig.

HelloGrayson · 2017-02-21T22:06:29Z

modules/rpc/yarpc.go

+	// Try to resolve a controller first
+	// TODO(alsam) use dig options when available, because we can overwrite the controller in case of multiple
+	// modules registering a controller.
+	if err := dig.Resolve(&module.controller); err != nil {


I'm confused by the term controller here... what exactly is it's responsibility?

There can be multiple modules that can can use YARPC, dispatcherController combines their configs in one and creates a single dispatcher, that a service will use.

Should we log this err here?

No, because it can be a valid error: no one registered a controller, and this is what we are going to do in the next line.

anuptalwalkar

Minor nits to resolve. Also can you update the title before merge?

anuptalwalkar · 2017-02-21T23:08:55Z

modules/rpc/yarpc_options.go

@@ -29,6 +29,7 @@ import (
 const (
 	_interceptorKey       = "yarpcUnaryInboundMiddleware"
 	_onewayInterceptorKey = "yarpcOnewayInboundMiddleware"
+	_graphInterceptorKey  = "yarpcDIGraph"


You don't need this now.

anuptalwalkar · 2017-02-22T00:04:52Z

modules/rpc/yarpc.go

+	// Try to resolve a controller first
+	// TODO(alsam) use dig options when available, because we can overwrite the controller in case of multiple
+	// modules registering a controller.
+	if err := dig.Resolve(&module.controller); err != nil {


Should we log this err here?

anuptalwalkar · 2017-02-22T00:05:23Z

modules/rpc/yarpc_test.go

@@ -25,6 +25,7 @@ import (

 	"go.uber.org/fx/service"

+	"fmt"


fmt should be with stdlib imports

anuptalwalkar · 2017-02-22T00:06:59Z

modules/rpc/thrift_test.go

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

 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/fx/dig"
+	"go.uber.org/yarpc"


nit: arrange imports.

coveralls · 2017-02-22T00:20:09Z

Changes Unknown when pulling 2619746 on diDispatcher into ** on master**.

glibsm reviewed Feb 21, 2017

View reviewed changes

madhuravi reviewed Feb 21, 2017

View reviewed changes

dmcaulay reviewed Feb 21, 2017

View reviewed changes

Alexandr Samylkin added 4 commits February 21, 2017 00:21

First version

a22ef16

Fix tests and resolve

e78bbc4

Fix comments

136a6eb

Store controller instead of di graph in module

a3d1048

ghost force-pushed the diDispatcher branch from 3f344aa to a3d1048 Compare February 21, 2017 08:22

Fix test comment

8bc5fb6

More details to a todo

00f640e

anuptalwalkar suggested changes Feb 21, 2017

View reviewed changes

Use a global DI

6270af9

HelloGrayson reviewed Feb 21, 2017

View reviewed changes

anuptalwalkar approved these changes Feb 22, 2017

View reviewed changes

Address nits

2619746

ghost changed the title ~~DI dispatcher~~ Register dispatcher in the default dig graph Feb 22, 2017

ghost merged commit b897b67 into master Feb 22, 2017

ghost deleted the diDispatcher branch February 22, 2017 01:11

This pull request was closed.

Register dispatcher in the default dig graph #304

Register dispatcher in the default dig graph #304

Conversation

ghost commented Feb 21, 2017 • edited by ghost Loading

mention-bot commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 21, 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

ghost Feb 21, 2017 • edited by ghost 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

coveralls commented Feb 21, 2017

coveralls commented Feb 21, 2017

coveralls commented Feb 21, 2017

coveralls commented Feb 21, 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

ghost Feb 21, 2017 • edited by ghost 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

coveralls commented Feb 21, 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

anuptalwalkar left a 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

coveralls commented Feb 22, 2017

ghost commented Feb 21, 2017 •

edited by ghost

Loading

ghost Feb 21, 2017 •

edited by ghost

Loading

ghost Feb 21, 2017 •

edited by ghost

Loading