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 broker(s) which no longer exist in metadata #1645

Merged
merged 1 commit into from
Apr 7, 2020
Merged

Remove broker(s) which no longer exist in metadata #1645

merged 1 commit into from
Apr 7, 2020

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 19, 2020 03:38
@Stephan14 Stephan14 changed the title remove broker which is not exist in metadata Remove broker which is not exist in metadata Mar 19, 2020
@Stephan14
Copy link
Contributor Author

Hey @d1egoaz, I have found some error as follows:
image
And run test on my Mac, it show as follows:
image
I think Got err: kafka: client has run out of available brokers to talk to is only one log and the code is right, so I delete this log

@Stephan14
Copy link
Contributor Author

Hey @bai @d1egoaz, I find other PR appear some error on CI, for example https://github.com/Shopify/sarama/pull/1640/checks?check_run_id=504088005, but it check successful. So how to solve the problem.

@d1egoaz
Copy link
Contributor

d1egoaz commented Mar 19, 2020

Thanks @Stephan14 for the new PR, so it seems the changes that you are introducing are indeed changing/breaking some expectation in some tests. We'll need to 👀 what's going on.

@Stephan14
Copy link
Contributor Author

Hey @d1egoaz , how about this problem is going? Can I merge this PR to master? The failures is as follow:
image

@dnwe
Copy link
Collaborator

dnwe commented Apr 6, 2020

@Stephan14 ignore those annotations, they don't actually tell you what went wrong in your build. The issue is that broken tests are running for a full 6 minutes pumping out logs, so the GitHub actions log file grows to 200MB and is hard to then read.

On master this test completes in less than a second:

go test -run '^TestConsumerRebalancingMultiplePartitions$' -v -timeout 5s

But on your branch this test is never meeting its exit condition and continues for 6 minutes before timing out. I can reproduce locally. Can you investigate on your machine and see if you can fix your changes for this?

@Stephan14
Copy link
Contributor Author

Stephan14 commented Apr 7, 2020

@dnwe , oh, sorry. I have fixed the problem and run go test on my mac successfully as follows:
image
But it runs failed in some CI as follows:
image
How to fix it? And @d1egoaz Dou you know ?

@dnwe dnwe closed this Apr 7, 2020
@dnwe dnwe reopened this Apr 7, 2020
@dnwe dnwe changed the title Remove broker which is not exist in metadata Remove broker(s) which no longer exist in metadata Apr 7, 2020
@dnwe
Copy link
Collaborator

dnwe commented Apr 7, 2020

@Stephan14 thanks for the fix. Looks like go actions had some issue fetching dependencies, but all the re-runs are green. Merging

@dnwe dnwe merged commit 2d3d6cd into IBM:master Apr 7, 2020
@Stephan14 Stephan14 deleted the bugfix/remove_invalid_broker branch April 8, 2020 02:36
@brenley
Copy link

brenley commented Apr 15, 2020

Hello! Just curious if this will be in the next release and when this is slated for? We have observed this issue and are deciding on whether to wait or deviate from release pattern and just pull in master.

@dnwe
Copy link
Collaborator

dnwe commented Apr 15, 2020

@brenley theres a few other open issues and it would be good to get some testing done against the final GA of kafka 2.5 but I'd imagine we'd be able to release a semver bump within a short period of time

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.

4 participants