Add topic interfaces #4
Conversation
topic/service.go
Outdated
| return nil | ||
| } | ||
| t, err := NewTopicFromValue(value) | ||
| if err != nil { |
There was a problem hiding this comment.
Why do we want to ignore errors here (or rather in the Watch method on the interface)?
There was a problem hiding this comment.
Basically this happens when there is an invalid topic proto in KV and the user could probably only treat that case as if the topic was deleted from KV, so it returns a nil Topic here.
There was a problem hiding this comment.
Can we return an exported error so user can make that decision instead of the library making the decision for them?
There was a problem hiding this comment.
sure can expose error through the interface
topic/types.go
Outdated
| // Watch watches the updates of a topic. | ||
| type Watch interface { | ||
| C() <-chan struct{} | ||
| Get() Topic |
There was a problem hiding this comment.
As mentioned in a previous comment - if why shouldn't we allow for Get() to propagate errors? Especially if this interface is to be used in applications where there can be errors in retrieving values.
There was a problem hiding this comment.
This is to keep the convention as other Watches, like m3x or m3cluster, we could return Topic, error here, but user probably can't do much with the error update as well. I don't feel strongly either way.
topic/types.go
Outdated
|
|
||
| // Service provides accessibility to topics. | ||
| type Service interface { | ||
| // Get returns the topic for the given name. |
There was a problem hiding this comment.
Nit: Might be worth adding in the comment that the int param is version.
| ConsumerServices() []ConsumerService | ||
|
|
||
| // SetConsumerServices sets the consumers of the topic. | ||
| SetConsumerServices(value []ConsumerService) Topic |
There was a problem hiding this comment.
Do we want to include an AddConsumerService method so that users dont have to pass in an entire list of all the existing consumer services if they just want to append one service to the end of a list.
Also presumably this topic metadata is stored in KV, however from what I can tell this function does not take a version so we can not leverage check and set.
There was a problem hiding this comment.
The CAS is not an issue, when you do a get you will get the version and the topic, you manipulate the topic then you can check and set with old version you got. For AddConsumerService, I'm on the fence, I guess we can do it for both the add and remove cases, maybe I'll add it when I really need it. Right now I need the Set function only for tests.
There was a problem hiding this comment.
Added version anyways so the user of topic can log version if needed
topic/topic.go
Outdated
| SetConsumerServices(css), nil | ||
| } | ||
|
|
||
| func (t *topic) Name() string { |
There was a problem hiding this comment.
MEGA-NIT: for the getters we could use a non-pointer receiver.
There was a problem hiding this comment.
Not making much difference in this case, I used to be a fan of non-pointer receiver as well but the convention in m3db repos is just using pointer receiver in many cases.
| return t.name | ||
| } | ||
|
|
||
| func (t *topic) SetName(value string) Topic { |
There was a problem hiding this comment.
NIT: this is just a naming thing. It seems like this function would set the name of the topic, however it returns a new topic with the name set, the original topic name is unchanged. I might point this out in a comment or something.
There was a problem hiding this comment.
Yeah this is a common pattern used in m3db repos, almost all set functions returns a new one unless it's too expensive to create a new one. I think your comment is valid, maybe we should point that out to the interfaces that is changing the original object.
| }, nil | ||
| } | ||
|
|
||
| func (s *service) Get(name string) (Topic, int, error) { |
There was a problem hiding this comment.
Comments on public functions? I would assume they are similar to those defined in the interface in the types.go file, but it would be nice to have those here too.
There was a problem hiding this comment.
Usually we don't repeat those comments if they are not changed from the interface.
topic/service.go
Outdated
| return &w{w: watch} | ||
| } | ||
|
|
||
| type w struct { |
There was a problem hiding this comment.
for consistency can we name the struct a lowercase version of the interface it implements.
| return NewWatch(w), nil | ||
| } | ||
|
|
||
| func key(name string) string { |
There was a problem hiding this comment.
I'd like to have it for now, havn't finally decided how the key should look like yet, so that I just need to change this one function if I want to change all the key usages.
There was a problem hiding this comment.
In that case do you want to define a keyType and call this newKey(name string) keyType?
There was a problem hiding this comment.
Feels like an overkill to create a keyType which is just an alias of string type, but can do if you feel strongly
topic/service.go
Outdated
| return &watch{w: w} | ||
| } | ||
|
|
||
| type watch struct { |
There was a problem hiding this comment.
Do you want to just embed it?
topic/service.go
Outdated
| return nil | ||
| } | ||
| t, err := NewTopicFromValue(value) | ||
| if err != nil { |
There was a problem hiding this comment.
Can we return an exported error so user can make that decision instead of the library making the decision for them?
topic/topic.go
Outdated
| case topicpb.ConsumptionType_REPLICATED: | ||
| return Replicated, nil | ||
| } | ||
| var c ConsumptionType |
There was a problem hiding this comment.
Should we define an Unknown consumption type? Otherwise this will just be Shared by default.
topic/topic.go
Outdated
| @@ -92,6 +97,16 @@ func (t *topic) SetConsumerServices(value []ConsumerService) Topic { | |||
| return &newt | |||
| } | |||
|
|
|||
| func (t *topic) Version() int { | |||
There was a problem hiding this comment.
Hm how is the version useful? Seems this is something that shouldn't be exposed to the users of this library no?
There was a problem hiding this comment.
Mainly for logging reasons, so that user can log that it's processing topic with version X
There was a problem hiding this comment.
Not sure if users of this library need to know that the topic has versions, but okay.
Add topic interfaces
@xichen2020 @jeromefroe @schallert