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

fix when consul is used as a name resolution component problem #1668

Closed
wants to merge 7 commits into from
Closed

fix when consul is used as a name resolution component problem #1668

wants to merge 7 commits into from

Conversation

shiling02404
Copy link
Contributor

Please reference the issue this PR will close:

#1513
#1199

…d cannot achieve load balancing, and the dapr started later will overwrite the previous dapr problem
@shiling02404 shiling02404 requested review from a team as code owners April 13, 2022 11:41
@@ -501,7 +501,7 @@ func TestGetConfig(t *testing.T) {
assert.Equal(t, 1, len(actual.Registration.Checks))
check := actual.Registration.Checks[0]
assert.Equal(t, "Dapr Health Status", check.Name)
assert.Equal(t, "daprHealth:test-app", check.CheckID)
Copy link
Member

Choose a reason for hiding this comment

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

why are you commenting out this test? Please provide a new test for your change instead if this test isn't compatible with your change.

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

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

Add or update test please. Don't forget to sign-off on your commit.

@shubham1172
Copy link
Member

Please fix the linter issues.

Interval: "15s",
HTTP: fmt.Sprintf("http://%s:%s/v1.0/healthz", host, httpPort),
DeregisterCriticalServiceAfter: "10m",
Copy link
Member

Choose a reason for hiding this comment

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

How was this value chosen?

Copy link
Member

@shubham1172 shubham1172 Apr 21, 2022

Choose a reason for hiding this comment

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

Should this be configurable? (with a sane default in place)

@yaron2
Copy link
Member

yaron2 commented Apr 28, 2022

ping @shiling02404.

@yaron2
Copy link
Member

yaron2 commented May 23, 2022

ping @shiling02404

@shiling02404
Copy link
Contributor Author

#1793

@shiling02404 shiling02404 deleted the feature-consul branch June 20, 2022 02:14
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