feat: support Consul as ServiceRegistry #666
Conversation
github-actions
bot
added
the
size/L
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #666 +/- ##
==========================================
- Coverage 88.16% 88.13% -0.04%
==========================================
Files 119 119
Lines 5763 5806 +43
==========================================
+ Hits 5081 5117 +36
- Misses 492 498 +6
- Partials 190 191 +1 ☔ View full report in Codecov by Sentry. |
| } | ||
| services, meta, err := reg.client.consulCatalog.Services(q) | ||
| if err != nil { | ||
| reg.logger.Errorf("failed to get services, err: %v", err) |
There was a problem hiding this comment.
Add a sleep and skip this turn would be better? So we can retry after some time.
| //} | ||
| defer ticker.Stop() | ||
| q := &consulapi.QueryOptions{ | ||
| WaitTime: dur, |
There was a problem hiding this comment.
Why don't we add Datacenter to this like what we do in fetchAllServices?
| return nil, nil | ||
| if err != nil { | ||
| reg.logger.Errorf("failed to get service, err: %v", err) | ||
| return nil |
There was a problem hiding this comment.
IMHO, we can pass the err out and make the Start fail if we fail to fetchAllServices. This can detect error like configuring a wrong consul address.
| select { | ||
| case <-reg.done: | ||
| reg.logger.Infof("stop refreshing services") | ||
| return | ||
|
|
||
| //wait to retry |
There was a problem hiding this comment.
We should retry when sync fails, not when the registry is done.
| return | ||
|
|
||
| //wait to retry | ||
| time.Sleep(SleepTime) |
There was a problem hiding this comment.
Would be better to use the configured duration as the sleep time, so it could be adaptable. For example, sleep N seconds when the WaitTime is N.
| @@ -127,3 +163,65 @@ func TestRefresh(t *testing.T) { | |||
| assert.Len(t, reg.softDeletedServices, 1) | |||
|
|
|||
| } | |||
|
|
|||
| func TestGenerateServiceEntry(t *testing.T) { | |||
| host := "test.default-group.public.earth.nacos" | |||
There was a problem hiding this comment.
Better use consul host in the consul test
| if err != nil { | ||
| return err | ||
| } | ||
| if reg.client == nil { |
There was a problem hiding this comment.
So far the Start would be called only one. We can skip the reg.client == nil check?
| @@ -235,3 +263,46 @@ func (reg *Consul) refresh(services map[string][]string) { | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| func (reg *Consul) generateServiceEntry(host string, services []model.SubscribeService) *registry.ServiceEntryWrapper { | |||
There was a problem hiding this comment.
This method is copied from Nacos and is not used in Consul?
| } | ||
|
|
||
| err := reg.Start(config) | ||
| assert.NoError(t, err) | ||
| Convey("Test Start method", t, func() { |
There was a problem hiding this comment.
Is there a strong reason to introduce a new testing library and write tests in another style?
| assert.NoError(t, err) | ||
| Convey("Test Start method", t, func() { | ||
|
|
||
| patches := gomonkey.ApplyMethod(reflect.TypeOf(reg), "FetchAllServices", func(_ *Consul, client *Client) (map[consulService]bool, error) { |
There was a problem hiding this comment.
We don't need to make this method public. See
ref #259
It's the part of refresh the Consul services