feat: add master logs endpoint #988
Conversation
| total := a.m.logs.Len() | ||
| offset := int(req.Offset) | ||
| if req.Offset < 0 { | ||
| offset = total + offset | ||
| if offset < 0 { | ||
| offset = 0 | ||
| } | ||
| } | ||
|
|
||
| limit := -1 | ||
| if req.Limit != 0 { | ||
| limit = int(req.Limit) | ||
| if limit > total-offset { | ||
| limit = total - offset | ||
| } | ||
| } |
There was a problem hiding this comment.
non-blocking: can we pull both of these blocks into helper functions that help explain what's going on (by having nice function names)?
master/internal/api_master.go
Outdated
| func (a *apiServer) MasterLogs( | ||
| req *apiv1.MasterLogsRequest, resp apiv1.Determined_MasterLogsServer) error { | ||
| if err := grpc.ValidateRequest( | ||
| func() (bool, string) { return req.Limit >= 0, "Limit must be >= 0" }, |
There was a problem hiding this comment.
should we also validation offset < a.m.logs.Len() (sort of how we set offset = 0 if the negative offset was too large).
There was a problem hiding this comment.
I went back and forth on this. Because the offset may be valid in the future, all the logging endpoints will accept offset that are not yet valid.
master/internal/api_master.go
Outdated
| for _, log := range a.m.logs.Entries(offset, -1, limit) { | ||
| offset += 1 | ||
| limit -= 1 | ||
| if err := resp.Send(log.Proto()); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if !req.Follow || limit == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
If we tail the logs, we'll overflow limit eventually and after ~ 2 * math.MaxInt64 exit because limit == 0 on line 62.
There was a problem hiding this comment.
Might should fix this by adding an
else if req.Follow {
limit = -1
}
somewhere?
|
closing in favor of #1007 |
Description
Test Plan
Commentary (optional)