Skip to content
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

[SCB-1301] Abstract control flow from serf/etcd/grpc #556

Merged
merged 3 commits into from Jun 12, 2019

Conversation

ChinX
Copy link
Contributor

@ChinX ChinX commented Jun 11, 2019

Abstract control flow from serf/etcd/grpc.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [SCB-XXX] Fixes bug in ApproximateQuantiles, where you replace SCB-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run go build go test go fmt go vet to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@coveralls
Copy link

coveralls commented Jun 11, 2019

Coverage Status

Coverage decreased (-0.07%) to 60.799% when pulling f270ca4 on ChinX:syncer into d7414f0 on apache:master.

serfAgent := a.Agent.Serf()
if serfAgent != nil {
for _, member := range serfAgent.Members() {
fmt.Println("member: ", member.Tags[tagKeyCluster])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change fmt to log ?

s.waitQuit(ctx)
s.servicecenter.SetStorage(s.etcd.Storage())

s.agent.RegisterEventHandler(s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Handler is same with the one in serf/agent.go, but they responsible for different event, I think we should add some comments to them at lease.

@@ -124,3 +163,37 @@ func (a *Agent) UserEvent(name string, payload []byte, coalesce bool) error {
func (a *Agent) Query(name string, payload []byte, params *serf.QueryParam) (*serf.QueryResponse, error) {
return a.Agent.Query(name, payload, params)
}

func (a *Agent) retryJoin(ctx context.Context) (err error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to give some comments here to tell what is retryJoin 's repsonsiblity?

"github.com/apache/servicecomb-service-center/syncer/plugins"
"github.com/apache/servicecomb-service-center/syncer/serf"
"github.com/apache/servicecomb-service-center/syncer/servicecenter"
)

var stopChanErr = errors.New("stopped syncer by stopCh")

type moduleServer interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to give some comments to tell why moduleServer interface here?

@@ -21,10 +21,11 @@ import (
"errors"
"testing"

"github.com/apache/servicecomb-service-center/syncer/pkg/mock/mocksotrage"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line, delete?

@zenlint
Copy link
Member

zenlint commented Jun 11, 2019

@ChinX It is LGTM, but I think you can add some comments to the key progress first. Please let me know once you finish the comments.
By the way, I change the name of PR to make it more clear

@zenlint zenlint changed the title [SCB-1301] Use goroutine to control the startup process [SCB-1301] Abstract control flow from serf/etcd/grpc Jun 11, 2019
@codecov-io
Copy link

codecov-io commented Jun 11, 2019

Codecov Report

Merging #556 into master will decrease coverage by 0.14%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #556      +/-   ##
=========================================
- Coverage   58.55%   58.4%   -0.15%     
=========================================
  Files         201     201              
  Lines       15257   15315      +58     
=========================================
+ Hits         8934    8945      +11     
- Misses       5701    5750      +49     
+ Partials      622     620       -2
Impacted Files Coverage Δ
syncer/servicecenter/servicecenter.go 95% <100%> (+0.12%) ⬆️
syncer/serf/agent.go 39.79% <26.47%> (-38.47%) ⬇️
syncer/serf/config.go 53.33% <40%> (-4.36%) ⬇️
syncer/config/config.go 85.18% <80%> (-3.28%) ⬇️
server/service/instance.go 70.94% <0%> (+0.15%) ⬆️
server/service/tag.go 67.95% <0%> (+0.55%) ⬆️
server/notify/websocket.go 81.14% <0%> (+0.57%) ⬆️
pkg/tlsutil/tlsutil.go 74.52% <0%> (+0.94%) ⬆️
scctl/pkg/plugin/diagnose/compare_holder.go 96.15% <0%> (+1.28%) ⬆️
server/service/util/heartbeat_util.go 76.47% <0%> (+5.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7414f0...f270ca4. Read the comment docs.

@zenlint
Copy link
Member

zenlint commented Jun 12, 2019

LGTM

@zenlint zenlint merged commit 605d0da into apache:master Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants