Rename RPC module to YARPC #325

ascandella · 2017-02-23T20:30:23Z

Fixes #267

mention-bot · 2017-02-23T20:30:24Z

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

bufdev · 2017-02-23T20:35:18Z

coveralls · 2017-02-23T20:35:27Z

Coverage remained the same at 92.029% when pulling 5bb83c4 on rename-rpc-to-yarpc into ef394bd on master.

HelloGrayson

Thanks @sectioneight - this will make it more obvious to users what it is they are actually configuring.

ascandella · 2017-02-23T20:37:18Z

So, I think I'm going to hold off on merging this since @peter-edge and @akshayjshah both have some gigantic PRs in-flight that they'd like to land soon, and this would cause merge conflicts. Once those are merged, I'll fix the conflicts and merge this.

glibsm · 2017-02-23T21:18:47Z

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog

+## v1.0.0-beta2 (unreleased)
+
+* [Breaking] Rename `modules/rpc` to `modules/yarpc`


I think we should leave a note about the yaml change that needs to happen. I think that's the way YARPC has been writing changelog notes - with an action that needs to be taken, or more detailed information of what the rename actually entails.

What yaml change?

The key should be changed to yarpc. @sectioneight you need to just remove the default naming in the ThriftModule function (where it sets the name to "rpc"), see my comment on yarpc.go

yes, the yaml key also changes from "rpc" to "yarpc". It is not immediately obvious from this changelog entry that service owners should update their config

I was hoping it was left on purpose..

bufdev · 2017-02-23T21:29:19Z

modules/yarpc/yarpc.go

 	mi service.ModuleCreateInfo,
 	reg registerServiceFunc,
 	options ...modules.Option,
-) (*YARPCModule, error) {
+) (*Module, error) {
 	name := "yarpc"


So on my "no default names" thing - note that for newModule, it's defaulting to yarpc, while in ThriftModule in thrift.go it's defaulting to rpc @madhuravi

Thanks for catching that Peter. Ot should always be "yarpc"

I looked up current code and don't see a separate name being set for thrift. Maybe I'm missing something? Which file are you looking at Peter?

ascandella · 2017-02-23T21:33:00Z

This PR doesn't change the YAML behavior though, right? I can add a code change that also changes the module config name, but it's not implicit based on the package name currently, right?

…

On Thu, Feb 23, 2017 at 1:31 PM Glib Smaga ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In CHANGELOG.md <#325 (comment)>: > @@ -1,5 +1,9 @@ # Changelog +## v1.0.0-beta2 (unreleased) + +* [Breaking] Rename `modules/rpc` to `modules/yarpc` yes, the yaml key also changes from "rpc" to "yarpc". It is not immediately obvious from this changelog entry that service owners should update their config — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#325 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF7P8IQ0_VnlPPr0GrG8w-xSoUzIcohks5rffq6gaJpZM4MKdlL> .

bufdev · 2017-02-23T21:34:31Z

This PR *should* change the yaml behavior per #267 though, and per a convo we had earlier (where package names should match default module names) On Thu, Feb 23, 2017 at 10:33 PM, Aiden Scandella <notifications@github.com> wrote:

…

This PR doesn't change the YAML behavior though, right? I can add a code change that also changes the module config name, but it's not implicit based on the package name currently, right? On Thu, Feb 23, 2017 at 1:31 PM Glib Smaga ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In CHANGELOG.md > <#325 (comment)>: > > > @@ -1,5 +1,9 @@ > # Changelog > > +## v1.0.0-beta2 (unreleased) > + > +* [Breaking] Rename `modules/rpc` to `modules/yarpc` > > yes, the yaml key also changes from "rpc" to "yarpc". It is not > immediately obvious from this changelog entry that service owners should > update their config > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#325 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe- auth/AAF7P8IQ0_VnlPPr0GrG8w-xSoUzIcohks5rffq6gaJpZM4MKdlL> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#325 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AECGvEaIBff98GKDbVsCCKxzS1EZRtWZks5rffsNgaJpZM4MKdlL> .

ascandella · 2017-02-23T21:37:45Z

Makes sense. Will update.

…

On Thu, Feb 23, 2017 at 1:34 PM Peter Edge ***@***.***> wrote: This PR *should* change the yaml behavior per #267 though, and per a convo we had earlier (where package names should match default module names) On Thu, Feb 23, 2017 at 10:33 PM, Aiden Scandella < ***@***.***> wrote: > This PR doesn't change the YAML behavior though, right? I can add a code > change that also changes the module config name, but it's not implicit > based on the package name currently, right? > > On Thu, Feb 23, 2017 at 1:31 PM Glib Smaga ***@***.***> > wrote: > > > ***@***.**** commented on this pull request. > > ------------------------------ > > > > In CHANGELOG.md > > <#325 (comment)>: > > > > > @@ -1,5 +1,9 @@ > > # Changelog > > > > +## v1.0.0-beta2 (unreleased) > > + > > +* [Breaking] Rename `modules/rpc` to `modules/yarpc` > > > > yes, the yaml key also changes from "rpc" to "yarpc". It is not > > immediately obvious from this changelog entry that service owners should > > update their config > > > > — > > You are receiving this because you were mentioned. > > Reply to this email directly, view it on GitHub > > <#325 (comment)>, or mute > > the thread > > <https://github.com/notifications/unsubscribe- > auth/AAF7P8IQ0_VnlPPr0GrG8w-xSoUzIcohks5rffq6gaJpZM4MKdlL> > > . > > > > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#325 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AECGvEaIBff98GKDbVsCCKxzS1EZRtWZks5rffsNgaJpZM4MKdlL > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#325 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF7P5WcY3iYScNsTWd6drmoxsV__VTXks5rfft1gaJpZM4MKdlL> .

akshayjshah · 2017-02-24T00:31:26Z

So, I think I'm going to hold off on merging this since @peter-edge and @akshayjshah both have some gigantic PRs in-flight that they'd like to land soon, and this would cause merge conflicts. Once those are merged, I'll fix the conflicts and merge this.

🙏 Your mercy is appreciated.

Fixes #267

anuptalwalkar

this is going to fail make thriftsync :(

ascandella · 2017-03-01T23:05:59Z

@anuptalwalkar yea, I have an internal diff open that I've been waiting to land until this change is merged.

ghost · 2017-03-02T01:24:02Z

modules/yarpc/yarpc.go

 	moduleOptions := &moduleOptions{}
 	for _, option := range options {
 		if err := option(moduleOptions); err != nil {
 			return nil, err
 		}
 	}
-	module := &YARPCModule{
+	module := &Module{
 		host:        host,
 		statsClient: newStatsClient(host.Metrics()),
 		log:         ulog.Logger(context.Background()).With(zap.String("module", host.Name())),


Something fishy is going on the line 274: We are using host.Name to get yarpc config..

I think host.Name() is now scoped post-peter's-refactor to the module name? @peter-edge to confirm

coveralls · 2017-03-02T01:28:36Z

Coverage remained the same at 90.391% when pulling 6c5bf5d on rename-rpc-to-yarpc into a35fbb7 on master.

ascandella · 2017-03-02T01:29:31Z

gimme tip appears to be broken, unrelated to this PR. Build passes on 1.7 and 1.8. Merging with the team's blessing.

ghost approved these changes Feb 23, 2017

View reviewed changes

HelloGrayson approved these changes Feb 23, 2017

View reviewed changes

anuptalwalkar approved these changes Feb 23, 2017

View reviewed changes

glibsm reviewed Feb 23, 2017

View reviewed changes

bufdev reviewed Feb 23, 2017

View reviewed changes

Rename RPC module to YARPC

582d2db

Fixes #267

ascandella force-pushed the rename-rpc-to-yarpc branch from ae29af2 to 582d2db Compare March 1, 2017 22:21

madhuravi approved these changes Mar 1, 2017

View reviewed changes

anuptalwalkar approved these changes Mar 1, 2017

View reviewed changes

Rename package

6fb2165

ascandella added 2 commits March 1, 2017 17:08

Fix gendoc

f35f290

Fix lint and stutters

2285c6c

ghost reviewed Mar 2, 2017

View reviewed changes

Fix test

6c5bf5d

ascandella merged commit ef49c44 into master Mar 2, 2017

ascandella deleted the rename-rpc-to-yarpc branch March 2, 2017 01:30

ascandella mentioned this pull request Mar 2, 2017

Temporarily disable tip while gimme is broken in travis #346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename RPC module to YARPC #325

Rename RPC module to YARPC #325

ascandella commented Feb 23, 2017

mention-bot commented Feb 23, 2017

bufdev commented Feb 23, 2017

coveralls commented Feb 23, 2017 •

edited

HelloGrayson left a comment

ascandella commented Feb 23, 2017

glibsm Feb 23, 2017

ghost Feb 23, 2017

bufdev Feb 23, 2017

glibsm Feb 23, 2017

ghost Feb 23, 2017

bufdev Feb 23, 2017 •

edited

madhuravi Feb 23, 2017

madhuravi Feb 23, 2017

ascandella commented Feb 23, 2017 via email

bufdev commented Feb 23, 2017 via email

ascandella commented Feb 23, 2017 via email

akshayjshah commented Feb 24, 2017

anuptalwalkar left a comment

ascandella commented Mar 1, 2017

ghost Mar 2, 2017

ascandella Mar 2, 2017

coveralls commented Mar 2, 2017 •

edited

ascandella commented Mar 2, 2017

Rename RPC module to YARPC #325

Rename RPC module to YARPC #325

Conversation

ascandella commented Feb 23, 2017

mention-bot commented Feb 23, 2017

bufdev commented Feb 23, 2017

coveralls commented Feb 23, 2017 • edited

HelloGrayson left a comment

Choose a reason for hiding this comment

ascandella commented Feb 23, 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

bufdev Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascandella commented Feb 23, 2017 via email

bufdev commented Feb 23, 2017 via email

ascandella commented Feb 23, 2017 via email

akshayjshah commented Feb 24, 2017

anuptalwalkar left a comment

Choose a reason for hiding this comment

ascandella commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 2, 2017 • edited

ascandella commented Mar 2, 2017

coveralls commented Feb 23, 2017 •

edited

bufdev Feb 23, 2017 •

edited

coveralls commented Mar 2, 2017 •

edited