add config of elasticsearch's user and password #104
Conversation
it looks that you have already support for multiple Primary key, but forget remove the code.
…icsearch Conflicts: river/river.go
|
It seems that my understand for error is different from your's |
| if err != nil { | ||
| return nil, errors.Trace(err) | ||
| } | ||
| if len(c.User) > 0 && len(c.Password) > 0 { | ||
| req.SetBasicAuth(c.User, c.Password) |
There was a problem hiding this comment.
can we meet the case that user is not empty but the password is?
river/river.go
Outdated
| @@ -66,7 +66,8 @@ func NewRiver(c *Config) (*River, error) { | |||
| return nil, errors.Trace(err) | |||
| } | |||
|
|
|||
| r.es = elastic.NewClient(r.c.ESAddr) | |||
| r.es = elastic.NewClient(r.c.ESAddr, r.c.ESUser, r.c.ESPassword) | |||
There was a problem hiding this comment.
please pass a config structure for client now.
|
@siddontang @EagleChen |
|
HI @WangXiangUSTC @EagleChen It makes more sense. |
|
@siddontang you means add more test case? Can you say a little more detail |
|
I say use another PR to check the status code first, then you can the password support later. I don't want to remove current tests if possible. For password, we can use other new tests especially. |
|
@siddontang I will try recently |
|
@siddontang Finally I decide to judge http response code after send http request, instead of return error when code is not 200. |
elastic/client.go
Outdated
|
|
||
| // if index doesn't exist, will get 404 not found error, create index first | ||
| if err != nil { | ||
| if r.Code != http.StatusOK { |
There was a problem hiding this comment.
should we check 404 here directly?
river/sync.go
Outdated
| @@ -439,7 +439,7 @@ func (r *River) doBulk(reqs []*elastic.BulkRequest) error { | |||
| if resp, err := r.es.Bulk(reqs); err != nil { | |||
| log.Errorf("sync docs err %v after binlog %s", err, r.canal.SyncedPosition()) | |||
| return errors.Trace(err) | |||
| } else if resp.Errors { | |||
| } else if resp.Code >= 300 || resp.Code < 200 || resp.Errors { | |||
There was a problem hiding this comment.
resp.Code / 100 == 2 may be better?
river/river_test.go
Outdated
| @@ -198,7 +199,11 @@ func (s *riverTestSuite) testElasticGet(c *C, id string) *elastic.Response { | |||
| docType := "river" | |||
|
|
|||
| r, err := s.r.es.Get(index, docType, id) | |||
| c.Assert(err, IsNil) | |||
| if err != nil { | |||
| r := new(elastic.Response) | |||
There was a problem hiding this comment.
why return a response with Found=false here?
|
@siddontang OK， I will check carefully tomorrow. |
elastic/client.go
Outdated
| @@ -214,7 +228,7 @@ func (c *Client) CreateMapping(index string, docType string, mapping map[string] | |||
| return errors.Trace(err) | |||
| } | |||
|
|
|||
| // index doesn't exist, create index first | |||
| // if index doesn't exist, will get 404 not found error, create index first | |||
| if r.Code != http.StatusOK { | |||
There was a problem hiding this comment.
maybe we can check 404 here directly and return an error for other invalid statuses.
|
@siddontang I have modify the code. |
elastic/client.go
Outdated
| _, err = c.Do("PUT", reqUrl, nil) | ||
|
|
||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| } else if r.Code != http.StatusOK { | ||
| return errors.Trace(err) |
There was a problem hiding this comment.
seem the error may be nil here.
elastic/client.go
Outdated
| _, err = c.Do("PUT", reqUrl, nil) | ||
|
|
||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| } else if r.Code != http.StatusOK { | ||
| return errors.New(resp.Status) |
There was a problem hiding this comment.
I guess compiler may fail here.
|
Thanks @WangXiangUSTC |
Add config of elasticsearch's user and password,
and add judge of HTTP response。