-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
package sd (take 2) #276
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
package sd (take 2) #276
Conversation
13107e9 to
00dd213
Compare
examples/addsvc2/service.go
Outdated
| // Sum implements Service. | ||
| func (s basicService) Sum(_ context.Context, a, b int) (int, error) { | ||
| if a == 0 && b == 0 { | ||
| return 0, ErrTwoZeroes |
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.
maybe we should test for a "real" error (overflow) to signal people that even the simplest method probably has real exceptions / error conditions so they don't think to lightly about proper error handling for every method
|
something quick I came up with for the above mentioned comments... const (
intMax = 1<<31 - 1
intMin = -(intMax + 1)
maxLen = 102400
)
// Service Method Errors
var (
ErrIntOverflow = errors.New("integer overflow")
ErrMaxSizeExceeded = errors.New("result exceeds maximum size")
)
// Sum implements Service.
func (s basicService) Sum(_ context.Context, a, b int) (int, error) {
if (b > 0 && a > (intMax-b)) || (b < 0 && a < (intMin-b)) {
return 0, ErrIntOverflow
}
return a + b, nil
}
// Concat implements Service.
func (s basicService) Concat(_ context.Context, a, b string) (string, error) {
if len(a)+len(b) > maxLen {
return "", ErrMaxSizeExceeded
}
return a + b, nil
} |
| sumEndpoint = circuitbreaker.Gobreaker(gobreaker.NewCircuitBreaker(gobreaker.Settings{ | ||
| Name: "Sum", | ||
| Timeout: 30 * time.Second, | ||
| }))(sumEndpoint) |
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.
Can you use endpoint.Chain to compose this sequence?
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.
Absolutely, but I find this way — working from the inside out, with multiple expressions — more intuitive. (I added endpoint.Chain by request, at some point.)
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 also find using multiple expressions more intuitive. I argued for simplifying our APIs in that direction way back in ancient history (#14 (comment)). I wonder if the middleware abstraction has really proved worth it if the only real use of it (endpoint.Chain) is avoided because it is less intuitive.
For reference: endpoint.Chain was designed in the discussion on #95 and implemented in it's current form in #96.
d6337ca to
a906318
Compare
|
This is now ready for review, if anyone would care to give it a look. Refactoring addsvc and profilesvc took much longer than anticipated; the upside is I'm very happy with the state of those examples, now, and I think they will be very educating, especially to new users. |
package sd (take 2)
package sd (take 2)
Iteration on #235.
Package loadbalancer is deleted, replaced by package sd. What was previously loadbalancer.Publisher becomes sd.Subscriber, which I believe better reflects its purpose. That is, the sd/consul.Subscriber is subscribing to updates from Consul.
Additionally, we introduce a new type, sd.Registrar, responsible for advertising the lifecycle of the Go kit service into the corresponding service discovery system. That is, sd/consul.Registrar registers instances to Consul.
This is optional, as many users prefer to or must have their orchestration system manage entries in their service discovery system. But for those users who do self-registration, this will be useful. Addresses #167.
A few additional changes:
The checklist before this PR is mergeable:
Once this is merged I will file issues for: