Skip to content

Conversation

@abhi
Copy link
Contributor

@abhi abhi commented Jul 10, 2017

The debugs using the caller of method has been extremely useful
in faster debugging and understanding of flow. This commit uses context
to pass in the caller information instead of explicit method names.
The ideal way would have been to use the runtime package Caller function
to get the callers from the function stack instead of passing the method
explicitly. However since the caller tree need not be symmetric it would
make it ugly to retrieve the interested caller. Hencing moving the method
name passing to context which would make it cleaner and more readable.

Signed-off-by: Abhinandan Prativadi abhi@docker.com

loadBalancer := lb
networkID := nid
cleanupFuncs = append(cleanupFuncs, func() {
ctx := context.WithValue(context.Background(), callerCtxKey, "rmServiceBinding")

Choose a reason for hiding this comment

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

cleanupServiceBindings?

@fcrisciani
Copy link

@abhinandanpb Thanks makes sense, I was hoping there was something more structured to achieve that result.
There is only 1 error, the rest looks fine to me

@abhi abhi force-pushed the cleanup branch 4 times, most recently from 4e2d241 to ed21a4f Compare July 10, 2017 22:08
@fcrisciani
Copy link

LGTM, @sanimej ?

@abhi abhi force-pushed the cleanup branch 3 times, most recently from 863bd1c to 67d664f Compare July 11, 2017 18:33
The debugs using the caller of method has been extremely useful
in faster debugging and understanding of flow. This commit uses context
to pass in the caller information instead of explicit method names.
The ideal way would have been to use the runtime package Caller function
to get the callers from the function stack instead of passing the method
explicitly. However since the caller tree need not be symmetric it would
make it ugly to retrieve the interested caller. Hencing moving the method
name passing to context which would make it cleaner and more readable.

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>
@sanimej
Copy link

sanimej commented Jul 12, 2017

@abhinandanpb Having the caller's name does help to follow the debugs quicker. But using context is probably is not the right thing to do here.. from the context package,

Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions.

Using runtime.Callers will be better and also avoids the manual setup of the caller name all over the code. In most cases we are interested only in the immediate caller of the function. But if needed caller stack can be set to the required number. What do you mean by caller tree not being symmetric ?

@aaronlehmann
Copy link

@sanimej: I agree completely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants