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

Remove invalid broker by latest metadata #1635

Closed
wants to merge 2 commits into from
Closed

Remove invalid broker by latest metadata #1635

wants to merge 2 commits into from

Conversation

Stephan14
Copy link
Contributor

This pull request solves the problem: #1629. I find it only have update and add broker when update brokers by latest metadata, however there is no code of remove broker which if offline

@Stephan14 Stephan14 requested a review from bai as a code owner March 8, 2020 13:42
@ghost ghost added the cla-needed label Mar 8, 2020
@d1egoaz d1egoaz changed the title remove invalid broker by latest metadta Remove invalid broker by latest metadata Mar 10, 2020
@d1egoaz
Copy link
Contributor

d1egoaz commented Mar 10, 2020

Hey @Stephan14
Could you please take a look at the tests? (are failing), and speaking of tests, could you please add some tests for your changes.

I think this will solve some problems that I'm seeing in some of my apps when we do rollout deployments on our kafka clusters.

Thanks!

@Stephan14
Copy link
Contributor Author

Stephan14 commented Mar 14, 2020

hey @d1egoaz , I have fixed and add some tests and signed CLA, but it still show 7 falling checks. One of them shows pass on my mac as follow:

=== RUN   TestClientMetadataTimeout
--- PASS: TestClientMetadataTimeout (2.51s)
=== RUN   TestClientMetadataTimeout/timeout=250ms
    TestClientMetadataTimeout/timeout=250ms: client_test.go:702: Got err: kafka: client has run out of available brokers to talk to (Is your cluster reachable?) after waiting for: 313.008957ms
    --- PASS: TestClientMetadataTimeout/timeout=250ms (0.32s)
=== RUN   TestClientMetadataTimeout/timeout=500ms
    TestClientMetadataTimeout/timeout=500ms: client_test.go:702: Got err: kafka: client has run out of available brokers to talk to (Is your cluster reachable?) after waiting for: 508.929773ms
    --- PASS: TestClientMetadataTimeout/timeout=500ms (0.52s)
=== RUN   TestClientMetadataTimeout/timeout=750ms
    TestClientMetadataTimeout/timeout=750ms: client_test.go:702: Got err: kafka: client has run out of available brokers to talk to (Is your cluster reachable?) after waiting for: 833.03324ms
    --- PASS: TestClientMetadataTimeout/timeout=750ms (0.84s)
=== RUN   TestClientMetadataTimeout/timeout=900ms
    TestClientMetadataTimeout/timeout=900ms: client_test.go:702: Got err: kafka: client has run out of available brokers to talk to (Is your cluster reachable?) after waiting for: 826.578945ms
    --- PASS: TestClientMetadataTimeout/timeout=900ms (0.83s)
PASS

Process finished with exit code 0

But it shows as follow in github:
image

I can not understand why it show 7 falling checks

@ghost ghost removed the cla-needed label Mar 16, 2020
@Stephan14
Copy link
Contributor Author

I find some new errors:
image
How to fix it?

@d1egoaz
Copy link
Contributor

d1egoaz commented Mar 17, 2020

@Stephan14 not sure, I've been trying to re-run the tests and it's failing on the checkout action, nothing related to this change, it seems related to github actions, maybe @bai might know something about it

@Stephan14
Copy link
Contributor Author

Hey @bai , how to fix the problem for me? Thank you

@d1egoaz
Copy link
Contributor

d1egoaz commented Mar 18, 2020

@Stephan14 without closing this PR, can you try creating a new PR to see if that makes Github happy?

@Stephan14
Copy link
Contributor Author

Hey @d1egoaz I have created a new PR, #1645

@dnwe
Copy link
Collaborator

dnwe commented Apr 6, 2020

I think we can continue discussion in #1645

@dnwe dnwe closed this Apr 6, 2020
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.

3 participants