-
Notifications
You must be signed in to change notification settings - Fork 454
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 topic endpoints #1060
Add topic endpoints #1060
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1060 +/- ##
==========================================
- Coverage 77.46% 77.44% -0.02%
==========================================
Files 537 541 +4
Lines 45771 45896 +125
==========================================
+ Hits 35457 35546 +89
- Misses 8070 8096 +26
- Partials 2244 2254 +10
Continue to review full report at Codecov.
|
} | ||
|
||
func (h *AddHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
ctx := r.Context() |
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.
var()
ctx := r.Context() | ||
logger := logging.WithContext(ctx) | ||
|
||
var req admin.TopicAddRequest |
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.
var()
return | ||
} | ||
|
||
service, err := h.fn(h.client, r.Header) |
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 rename this serviceFn
return | ||
} | ||
|
||
t, err := service.Get(topicName(r)) |
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 t
be nil? Do you need to handle that
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.
t can't be nil when err==nil
client clusterclient.Client | ||
cfg config.Configuration | ||
|
||
fn serviceFn |
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 rename this serviceFn
} | ||
|
||
// Service gets a topic service from m3cluster client | ||
func Service(clusterClient clusterclient.Client, headers http.Header) (topic.Service, error) { |
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.
Do you need to accept headers?
r.HandleFunc(AddURL, logged(NewAddHandler(client, cfg)).ServeHTTP).Methods(AddHTTPMethod) | ||
} | ||
|
||
func topicName(r *http.Request) string { |
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.
Isnt the request a proto? How is this expected to be used. Can we just put it in the proto fields?
return | ||
} | ||
|
||
tn := topicName(r) |
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 just call this name
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
No description provided.