Module rewrite #275

bufdev · 2017-02-16T17:36:22Z

No description provided.

mention-bot · 2017-02-16T17:36:23Z

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

yutongp · 2017-02-16T17:59:26Z

service/module.go

+	Host
+	Items() map[string]interface{}
+	// TODO(pedge)
+	ConfigValue() config.Value


I thought Host already has Config

Ya I'm going to remove this for now

yutongp · 2017-02-16T18:02:07Z

modules/rpc/yarpc_options.go

-		return nil
-	}
+func WithInboundMiddleware(i ...middleware.UnaryInbound) service.ModuleOption {
+	return service.WithModuleItem(_interceptorKey, func(existing interface{}) interface{} {


I like this wrapping on write to module item.
maybe a similar wrapping for read as well?

What do you mean exactly? Wrap Items() access?

yutongp · 2017-02-16T18:04:46Z

service/module.go

+func WithModuleItem(key string, f func(interface{})interface{}) ModuleOption {
+	return func(o *moduleOption) error {
+		value, ok := o.items[key]
+		if ok {


nit:

if value, ok := o.items[key]; ok { o.items[key] = f(value) return nil } o.items[key] = f(nil) retur nil

ya whoops, sorry

dmcaulay

so much better 👏

dmcaulay · 2017-02-17T06:39:15Z

service/host.go

@@ -323,29 +304,34 @@ func (s *host) Stop(reason string, exitCode int) error {
 	return err
 }

-func (s *host) startModules() map[Module]error {
-	results := map[Module]error{}
+func (s *host) startModules() []error {


so this only happens once, right? how much time are we saving be starting these in parallel?

No idea, but this PR is just addressing the external interface, I think we can iterate on implementation some other time.

dmcaulay · 2017-02-17T06:40:10Z

service/host_mock.go

 	return &serviceCore{
 		authClient:     client,
-		configProvider: config.NewStaticProvider(nil),
+		configProvider: configProvider,
 		standardConfig: serviceConfig{
 			Name:        "dummy",


can we read the name from our config provider? i need this for some integration tests.

We can, but probably out of scope for this PR

dmcaulay · 2017-02-17T06:43:46Z

service/module.go

+	return m.name
+}
+
+func (m *moduleWrapper) Start() error {


can we remove locking if we single thread start/stop?

Ya we can, but I really think the locking is "proper".

okay. fine with it.

dmcaulay · 2017-02-17T06:44:49Z

service/module.go

+}
+
+func (mi *moduleInfo) Metrics() tally.Scope {
+	return mi.Host.Metrics().SubScope("modules").SubScope(mi.name)


is it more efficient to memoize this scope?

Yes, I think I have a TODO somewhere in this file to do that

ghost · 2017-02-17T17:50:14Z

modules/rpc/yarpc.go

@@ -65,7 +60,8 @@ var (
 	// that are stored together to create a shared dispatcher.
 	// The YARPC team advised it to be a 'singleton' to control
 	// the lifecycle of all of the in/out bound traffic.
-	_controller dispatcherController
+	_controller        dispatcherController
+	_controllerRunning bool


This global var is definitely not needed

ghost · 2017-02-17T17:51:30Z

modules/rpc/thrift.go

 type CreateThriftServiceFunc func(svc service.Host) ([]transport.Procedure, error)

 // ThriftModule creates a Thrift Module from a service func
-func ThriftModule(hookup CreateThriftServiceFunc, options ...modules.Option) service.ModuleCreateFunc {


Why are you removing options?

They are now on the service builder only

ghost · 2017-02-17T17:57:20Z

modules/task/backend.go

 	for msg := range b.bufQueue {
-		errorCh <- Run(context.Background(), msg)
+		// TODO(pedge): this was effectively not being handled and was a bug


It is not a bug, but a feature :) Seriously, this is a way to communicate with a sender about errors in function executions.

If you look into how this channel was read, this error would have never been read with the current service code

We do want to propagate these errors up so nothing to change here. Need to not drop them on the floor in module Start where this gets called.

bufdev · 2017-02-20T16:49:34Z

I've updated for the latest changes. Before continuing, if everyone could give a quick lookover of what has happened so far, that would be great - the next step is going to be a lot of test refactoring and documentation updates, and before doing that, it would be nice to have a general "OK" from what I have so far.

@sectioneight @dmcaulay @alsamylkin @madhuravi @yutongp @glibsm

madhuravi · 2017-02-20T18:51:10Z

service/builder.go

@@ -22,22 +22,26 @@ package service

 import "github.com/pkg/errors"

-// WithModules is a helper to create a service without any options
-func WithModules(modules ...ModuleCreateFunc) *Builder {


Why? I would prefer keeping WithModules.
1.
service.WithModule(
rpc,
uhttp,
task
)

service.WithModule(uhttp)
.WithModule(rpc)
.WithModule(task)

1 (the existing way) seems simpler and nicer than 2.

You can't do this anymore, WithModule takes more than one parameter for every module.

bufdev · 2017-02-20T18:56:32Z

The reason is because now WithModule takes options instead of the individual derivative functions, there are other ways to handle this but this seemed the easiest.

…

On Mon, Feb 20, 2017 at 7:51 PM Madhu Ravichandran ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In service/builder.go <#275 (comment)>: > @@ -22,22 +22,26 @@ package service import "github.com/pkg/errors" -// WithModules is a helper to create a service without any options -func WithModules(modules ...ModuleCreateFunc) *Builder { Why? I would prefer keeping WithModules. 1. service.WithModule( rpc, uhttp, task ) 1. service.WithModule(uhttp) .WithModule(rpc) .WithModule(task) 1 (the existing way) seems simpler and nicer than 2. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#275 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AECGvEza3O8QNDcwyW3PkurRd-EyHKRhks5reeCegaJpZM4MDUeB> .

dmcaulay

this is looking good to me. a lot of great work here.

coveralls · 2017-02-21T16:41:36Z

Changes Unknown when pulling d2a3a7b on module-rewrite into ** on master**.

madhuravi · 2017-02-21T16:46:44Z

modules/task/doc.go

@@ -48,7 +48,8 @@
 //   )


This file shouldn't be included?

? It changed from make gendoc.

coveralls · 2017-02-23T13:51:19Z

Coverage increased (+0.1%) to 92.139% when pulling c5af058 on module-rewrite into 8622d3c on master.

bufdev · 2017-02-23T13:51:29Z

Code coverage is back up

coveralls · 2017-02-23T14:08:39Z

Coverage increased (+0.1%) to 92.141% when pulling ca33b51 on module-rewrite into 8622d3c on master.

coveralls · 2017-02-24T13:01:10Z

Coverage increased (+0.08%) to 90.836% when pulling 5aad484 on module-rewrite into 732bd07 on master.

coveralls · 2017-02-24T13:01:10Z

Coverage increased (+0.08%) to 90.836% when pulling 5aad484 on module-rewrite into 732bd07 on master.

coveralls · 2017-03-01T13:17:02Z

Changes Unknown when pulling 12b58a2 on module-rewrite into ** on master**.

coveralls · 2017-03-01T13:55:45Z

Changes Unknown when pulling f0304ee on module-rewrite into ** on master**.

ascandella · 2017-03-01T21:28:53Z

👏 🎉

bufdev added 2 commits February 15, 2017 16:27

Add NopHostWithConfig function for testing

dcbc294

WIP module rewrite

0100d0d

yutongp reviewed Feb 16, 2017

View reviewed changes

dmcaulay reviewed Feb 17, 2017

View reviewed changes

More Module work

9d4b179

ghost reviewed Feb 17, 2017

View reviewed changes

bufdev mentioned this pull request Feb 20, 2017

Generic module package #244

Closed

merge

a25e1aa

bufdev changed the title ~~WIP Module rewrite~~ [WIP] [NOT READY FOR REVIEW] Module rewrite Feb 20, 2017

bufdev added 2 commits February 20, 2017 17:38

Finish merge hopefully

4dff7a6

Start fixes for modules rewrite

b0673fd

bufdev changed the title ~~[WIP] [NOT READY FOR REVIEW] Module rewrite~~ [WIP] Module rewrite Feb 20, 2017

madhuravi reviewed Feb 20, 2017

View reviewed changes

dmcaulay reviewed Feb 21, 2017

View reviewed changes

bufdev added 10 commits February 21, 2017 11:59

merge

8418ebf

Fix building of service

a6ac4dc

Fix building of service

90acd6c

Fix go build, but tests still failing

ea3b82f

Getting tests building

5fd1e23

Continue to fix testing

ec5b943

Continue to fix tests

cb17b4e

Tests passing

515c179

Fix lint

56eebe3

Update docs

d2a3a7b

Update thriftsync to use service.ModuleInfo

ea4700e

madhuravi reviewed Feb 21, 2017

View reviewed changes

Add more testing for yarpc options

c5af058

Wrap reading of ModuleInfo items

ca33b51

This was referenced Feb 23, 2017

Have separate package for rpc per library/encoding? #315

Closed

Change service.Option to use the manager struct #322

Merged

bufdev added 3 commits February 24, 2017 13:22

Merge

c0125bd

Tests passing

fdf389c

Fix examples

5aad484

bufdev added 5 commits March 1, 2017 13:21

Merge

eef5ef8

Fix after merge

4a37dc8

Make module options specific to rpc

89f73cc

Make module options specific to uhttp

12b58a2

Remove module items

08a90f0

bufdev added 2 commits March 1, 2017 14:46

Start removing ModuleInfo

a963a3f

Remove ModuleInfo

f0304ee

Cleanup

f2d5469

bufdev mentioned this pull request Mar 1, 2017

gRPC example #342

Closed

Merge

683f1d0

ascandella approved these changes Mar 1, 2017

View reviewed changes

glibsm approved these changes Mar 1, 2017

View reviewed changes

bufdev merged commit a35fbb7 into master Mar 1, 2017

bufdev deleted the module-rewrite branch March 1, 2017 21:28

madhuravi mentioned this pull request Mar 9, 2017

Set root name as default host.Name and separate the module name #385

Merged

Module rewrite #275

Module rewrite #275

Conversation

bufdev commented Feb 16, 2017

mention-bot commented Feb 16, 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

dmcaulay 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

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

Choose a reason for hiding this comment

bufdev commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufdev commented Feb 20, 2017 via email

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

coveralls commented Feb 23, 2017

bufdev commented Feb 23, 2017

coveralls commented Feb 23, 2017

coveralls commented Feb 24, 2017

coveralls commented Feb 24, 2017

coveralls commented Mar 1, 2017

coveralls commented Mar 1, 2017 • edited Loading

ascandella commented Mar 1, 2017

coveralls commented Mar 1, 2017 •

edited

Loading