-
Notifications
You must be signed in to change notification settings - Fork 929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add attribute method into Invocation and RpcInvocation #537
Add attribute method into Invocation and RpcInvocation #537
Conversation
protocol/invocation/rpcinvocation.go
Outdated
func (r *RPCInvocation) AttributeByKey(key string, defaultValue interface{}) interface{} { | ||
r.lock.RLock() | ||
defer r.lock.RUnlock() | ||
if r.attributes == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after init this map in newRPCInvocation, u can delete this if condition clause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two methods about NewRPCInvocation . Another one is NewRPCInvocationWithOptions. We cannot make it init it well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could init the attributes in NewRPCInvocationWithOptions too. The attribute map is empty. What's your concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flycash can not agree with u more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvictory pls improve NewRPCInvocationWithOptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added WithAttributes function for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvictory pls delete WithAttributes and modify NewRPCInvocationWithOptions
Codecov Report
@@ Coverage Diff @@
## develop #537 +/- ##
===========================================
- Coverage 67.20% 66.46% -0.75%
===========================================
Files 174 184 +10
Lines 9261 9712 +451
===========================================
+ Hits 6224 6455 +231
- Misses 2432 2617 +185
- Partials 605 640 +35
Continue to review full report at Codecov.
|
protocol/invocation/rpcinvocation.go
Outdated
func (r *RPCInvocation) AttributeByKey(key string, defaultValue interface{}) interface{} { | ||
r.lock.RLock() | ||
defer r.lock.RUnlock() | ||
if r.attributes == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could init the attributes in NewRPCInvocationWithOptions too. The attribute map is empty. What's your concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Some parameter need to be used in the internal process. For example we need calculate the serialize type and store into attribute. If we don't store it and this serialize value is used by some function or method , we must calculate it in every function.