service config: add default method config support #3684
Conversation
|
|
amenzhinsky
force-pushed
the
default-method-config
branch
from
5c57934
to
ef23d5c
Compare
service_config.go
Outdated
| if j.Service == nil { | ||
| return "", false | ||
| return "", errIgnoreName |
There was a problem hiding this comment.
What if j.Service is nil, but j.Method is set to non-empty string?
There was a problem hiding this comment.
errIgnoreName will make it simply ignore the name just like the current implementation does.
https://github.com/grpc/grpc-proto/blob/dd2dca318eb197b96b60f22297871fb1ed862800/grpc/service_config/service_config.proto#L67 states service is required, so probably we should return the corresponding error instead of skipping malformed names, but I didn't want to introduce breaking changes (if it's a breaking change).
There was a problem hiding this comment.
ef23d5c#diff-bd49846390d90e2bd0f528a7a4c1e533R339-R341
I removed this block in the latest patch, let me know if it's not something you wanted me to do.
service_config.go
Outdated
| if j.Service == nil { | ||
| return "", false | ||
| return "", errMissingService |
There was a problem hiding this comment.
I think there's some inconsistency in service_config.proto.
service shouldn't be marked as required, and this line says not present is considered the same as "".
(Does the json library handle null correctly? Can you add a test? Thanks!)
j.Service == nil should be considered the same as *j.Service == "", and we check if method is set, as in the following if.
On whether this is a breaking change, this only affect configs where service is not set, and method is set to non-empty string. This is not an valid config anyway, I think failing those would be OK.
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh, right, that's great!
Did you fix this
j.Service == nil should be considered the same as *j.Service == "", and we check if method is set, as in the following if.
I saw you force pushed, but I can't find the fix for it.
There was a problem hiding this comment.
I think I finally got it, was a bit confused. Please check out the latest commit
amenzhinsky
force-pushed
the
default-method-config
branch
from
158b808
to
540eba3
Compare
menghanl
changed the title
Fixes #3473