Skip to content

[metricbeat] migrate zookeeper to reporter V2 with error handling#11659

Merged
fearful-symmetry merged 2 commits intoelastic:masterfrom
fearful-symmetry:migration/mb/reporterv2error/zookeeper
Apr 9, 2019
Merged

[metricbeat] migrate zookeeper to reporter V2 with error handling#11659
fearful-symmetry merged 2 commits intoelastic:masterfrom
fearful-symmetry:migration/mb/reporterv2error/zookeeper

Conversation

@fearful-symmetry
Copy link
Copy Markdown
Contributor

@fearful-symmetry fearful-symmetry commented Apr 4, 2019 •
edited by ruflin
Loading

see #11374

Got a little creative here, so we could also clean up the deprecated logging methods. If there's a better way to do that, please tell me.

@fearful-symmetry fearful-symmetry added Metricbeat Metricbeat Team:Integrations Label for the Integrations team technical_debt labels Apr 4, 2019
@fearful-symmetry fearful-symmetry self-assigned this Apr 4, 2019
@fearful-symmetry fearful-symmetry requested a review from a team April 4, 2019 19:51
ruflin
ruflin approved these changes Apr 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. Two minor comments.

metricbeat/module/zookeeper/mntr/data.go Outdated
Copy link
Copy Markdown
Contributor

@ruflin ruflin Apr 5, 2019

Choose a reason for hiding this comment

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

In the past we made the decision that we try not to use Warn and always use either Error or Info to make it clear if user action is needed or not. I suggest we change this to Info.

I'm aware this is not introduced by this PR, but as we changed the line stumbled over it. We can change it in this PR or follow up.

metricbeat/module/zookeeper/server/data_integration_test.go Outdated
Copy link
Copy Markdown
Contributor

@ruflin ruflin Apr 5, 2019

Choose a reason for hiding this comment

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

+1 on moving this.

metricbeat/module/zookeeper/server/server_integration_test.go Outdated
Copy link
Copy Markdown
Contributor

@ruflin ruflin Apr 5, 2019

Choose a reason for hiding this comment

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

I think we don't need this line?

Copy link
Copy Markdown
Contributor Author

@fearful-symmetry fearful-symmetry Apr 5, 2019

Choose a reason for hiding this comment

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

Yah, I'm...not sure why that was skipped?

@fearful-symmetry fearful-symmetry mentioned this pull request Apr 5, 2019
Closed
93 tasks
@fearful-symmetry
Copy link
Copy Markdown
Contributor Author

fearful-symmetry commented Apr 7, 2019

jenkins, test this

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Apr 8, 2019

I wonder if the test failures are actually related to the change. It says coredns failed but it seems zookeeper is one of the last one to stop. Will run the tests again.

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Apr 8, 2019

jenkins, test this

@fearful-symmetry
Copy link
Copy Markdown
Contributor Author

fearful-symmetry commented Apr 8, 2019

Still seeing this:
Exception: Timeout while waiting for healthy docker-compose services: coredns

A bunch of the windows tests seem to be timing out, which I assume is unrelated...

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Apr 9, 2019

@fearful-symmetry Can you rebase again now that the fix for coredns was merged?

@fearful-symmetry fearful-symmetry force-pushed the migration/mb/reporterv2error/zookeeper branch from 943ae74 to 99933e4 Compare April 9, 2019 13:53
@fearful-symmetry fearful-symmetry merged commit 9aca9e7 into elastic:master Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ruflin ruflin ruflin approved these changes

+1 more reviewer

@kaiyan-sheng kaiyan-sheng kaiyan-sheng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@fearful-symmetry fearful-symmetry

Labels

Metricbeat Metricbeat Team:Integrations Label for the Integrations team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fearful-symmetry @ruflin @kaiyan-sheng @alvarolobato