http reader support multiple prefix #678
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mockerzzz
force-pushed
the
prefix_socket_reader
branch
5 times, most recently
from
531a518
to
d05b58f
Compare
wonderflow
reviewed
Aug 6, 2018
mgr/dataflow_test.go
Outdated
| @@ -319,19 +319,21 @@ func Test_SendData(t *testing.T) { | |||
| "senders": senders, | |||
| } | |||
|
|
|||
| go func() { | |||
wonderflow
reviewed
Aug 6, 2018
reader/http/http_test.go
Outdated
| wg.Add(1) | ||
| go func() { | ||
| got, err := httpReader.ReadLine() | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
goroutine里面用到的变量，如果没有当作参数传进去，就会出现并发错误，此处要把httpReader和t当作参数传入函数。
wonderflow
reviewed
Aug 6, 2018
reader/http/http.go
Outdated
| @@ -136,11 +142,10 @@ func (r *Reader) Close() error { | |||
| return nil | |||
| } | |||
| log.Debugf("Runner[%v] %q daemon is stopping", r.meta.RunnerName, r.Name()) | |||
|
|
|||
| close(r.readChan) | |||
There was a problem hiding this comment.
这里在http 仍然在serve的时候就close channel，可能会导致panic， send on closed channel
mockerzzz
force-pushed
the
prefix_socket_reader
branch
3 times, most recently
from
88df7d1
to
150500c
Compare
wonderflow
reviewed
Aug 6, 2018
reader/http/http.go
Outdated
| r.server.Shutdown(context.Background()) | ||
| err := r.bufQueue.Close() | ||
| close(r.readChan) |
There was a problem hiding this comment.
这里要确认一下，如果http server在shutdown的之前，postData函数被阻塞了，比如数据来不及读导致channel传不进去数据，那么这里close会导致panic吗
mockerzzz
force-pushed
the
prefix_socket_reader
branch
from
150500c
to
3768624
Compare
wonderflow
reviewed
Aug 7, 2018
| r.readChan <- Details{ | ||
| Content: line, | ||
| Path: path, | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
尝试实际运行起来，然后用大量的请求去请求这个http reader，然后请求不要停，紧接着logkit关掉，看看现象 |
unknwon
suggested changes
Aug 7, 2018
reader/http/http.go
Outdated
| "github.com/qiniu/logkit/reader" | ||
| . "github.com/qiniu/logkit/utils/models" | ||
| "sync" |
|
|
||
| server *http.Server | ||
| } | ||
|
|
||
| func NewReader(meta *reader.Meta, conf conf.MapConf) (reader.Reader, error) { | ||
| address, _ := conf.GetStringOr(reader.KeyHTTPServiceAddress, reader.DefaultHTTPServiceAddress) | ||
| path, _ := conf.GetStringOr(reader.KeyHTTPServicePath, reader.DefaultHTTPServicePath) | ||
| paths := strings.Split(path, ",") |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mockerzzz
force-pushed
the
prefix_socket_reader
branch
from
3768624
to
6336754
Compare
wonderflow
reviewed
Aug 8, 2018
reader/http/http.go
Outdated
| if val[0] == '/' { | ||
| validPaths = append(validPaths, val) | ||
| } else { | ||
| log.Warnf("path[%v] not begin with '/',this path ignored", val) |
There was a problem hiding this comment.
这里直接错误返回吧，不用Warn，反正是初始配置的时候
mockerzzz
force-pushed
the
prefix_socket_reader
branch
from
6336754
to
87c42d4
Compare
wonderflow
reviewed
Aug 8, 2018
reader/http/http.go
Outdated
|
|
||
| server *http.Server | ||
| } | ||
|
|
||
| func NewReader(meta *reader.Meta, conf conf.MapConf) (reader.Reader, error) { | ||
| address, _ := conf.GetStringOr(reader.KeyHTTPServiceAddress, reader.DefaultHTTPServiceAddress) | ||
| path, _ := conf.GetStringOr(reader.KeyHTTPServicePath, reader.DefaultHTTPServicePath) | ||
| paths := strings.Split(path, ",") | ||
| for _, val := range paths { | ||
| if val[0] != '/' { |
There was a problem hiding this comment.
你这样写如果val是空就panic了，先trimeSpace，空的continue，然后检查 strings.HasPrefix()
mockerzzz
force-pushed
the
prefix_socket_reader
branch
from
87c42d4
to
2a94417
Compare
wonderflow
reviewed
Aug 8, 2018
reader/http/http.go
Outdated
|
|
||
| server *http.Server | ||
| } | ||
|
|
||
| func NewReader(meta *reader.Meta, conf conf.MapConf) (reader.Reader, error) { | ||
| address, _ := conf.GetStringOr(reader.KeyHTTPServiceAddress, reader.DefaultHTTPServiceAddress) | ||
| path, _ := conf.GetStringOr(reader.KeyHTTPServicePath, reader.DefaultHTTPServicePath) | ||
| paths := strings.Split(path, ",") | ||
| for _, val := range paths { | ||
| if !strings.HasPrefix(val, "/") { |
|
文档也对应更新一下 mars上 |
mockerzzz
force-pushed
the
prefix_socket_reader
branch
from
2a94417
to
488b480
Compare
unknwon
reviewed
Aug 8, 2018
reader/http/http.go
Outdated
| r.server.Shutdown(context.Background()) | ||
| err := r.bufQueue.Close() | ||
| //Note：确保不会往已关闭的channel中写入message |
There was a problem hiding this comment.
这里描述不对吧，应该是和 wg.Done 一个作用，因为是 wait 完才 close 的 channel
|
LGTM |
mockerzzz
force-pushed
the
prefix_socket_reader
branch
from
488b480
to
2462fc8
Compare
unknwon
approved these changes
Aug 8, 2018
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
文档：https://developer.qiniu.io/insight/manual/4771/http-reader?version=5b6a6cc393b8cf242ec5d94e