Separate thrift handler for history service #3646
Conversation
service/history/MockHandler.go
Outdated
| // Source: github.com/uber/cadence/service/history (interfaces: Handler) | ||
|
|
||
| // Package history is a generated GoMock package. | ||
| package history |
There was a problem hiding this comment.
Agree, updated to handler_mock.go
service/history/handler.go
Outdated
| } | ||
|
|
||
| // HandlerImpl is an implementation for history service independent of wire protocol | ||
| HandlerImpl struct { |
There was a problem hiding this comment.
Agree, no need to expose this. Updated.
service/history/handler.go
Outdated
| @@ -92,8 +137,8 @@ var ( | |||
| func NewHandler( | |||
| resource resource.Resource, | |||
| config *config.Config, | |||
| ) *Handler { | |||
| handler := &Handler{ | |||
| ) *HandlerImpl { | |||
There was a problem hiding this comment.
could we return interface here?
There was a problem hiding this comment.
As per Go guidelines we should return concrete implementations instead of interfaces.
There was a problem hiding this comment.
Thanks. Good to know about this
service/history/service.go
Outdated
| @@ -42,7 +42,7 @@ type Service struct { | |||
| resource.Resource | |||
|
|
|||
| status int32 | |||
| handler *Handler | |||
| handler *HandlerImpl | |||
There was a problem hiding this comment.
Just use the interface rather than the implementation?
There was a problem hiding this comment.
At this point handler also use other methods (Start, Stop,...) defined on HandlerImpl but not on Handler interface. I wanted to keep this interface lean and only contain actual handler methods, not the utilities like Start and Stop.
Alternatively we could separate them into another interface and use that here instead. But for the time being and do not see immediate benefit for that and I wanted to keep this diff simple and mechanical.
This is purely a mechanical refactoring that introduces additional ThriftHandler wrapper. For now it only forwards request to the original handler. Later on this will become a point for conversion between internal types and thrift types. Underlying handler will be independent of rpc protocol types. - Created Handler interface for history service - Previous Handler struct renamed to handlerImpl - Created ThriftHandler wrapper on top of Handler interface. At this point it only forward requests as is to underlying handler. - Created Mock for Handler interface. - Covered ThriftHandler with units tests. Test that requests are forwarded to underlying handler and response is returned as is. - Replaced handler registration. Instead of registering handlerImpl directly, wrap it with ThriftHandler and register it instead.
This is purely a mechanical refactoring that introduces additional ThriftHandler wrapper. For now it only forwards request to the original handler. Later on this will become a point for conversion between internal types and thrift types. Underlying handler will be independent of rpc protocol types. - Created Handler interface for history service - Previous Handler struct renamed to handlerImpl - Created ThriftHandler wrapper on top of Handler interface. At this point it only forward requests as is to underlying handler. - Created Mock for Handler interface. - Covered ThriftHandler with units tests. Test that requests are forwarded to underlying handler and response is returned as is. - Replaced handler registration. Instead of registering handlerImpl directly, wrap it with ThriftHandler and register it instead.
What changed?
Why?
This is purely a mechanical refactoring that introduces additional ThriftHandler wrapper. For now it only forwards request to the original handler. Later on this will become a point for conversion between internal types and thrift types. Underlying handler will be independent of rpc protocol types.
How did you test it?
Potential risks
None, purely mechanical change.