Skip to content

[Metricbeat] Migrate Ceph osd_tree to use ReporterV2 interface#11048

Merged
sayden merged 2 commits intoelastic:masterfrom
berfinsari:migration/mb/reportv2/ceph/osd_tree
Mar 13, 2019
Merged

[Metricbeat] Migrate Ceph osd_tree to use ReporterV2 interface#11048
sayden merged 2 commits intoelastic:masterfrom
berfinsari:migration/mb/reportv2/ceph/osd_tree

Conversation

@berfinsari
Copy link
Copy Markdown
Contributor

@berfinsari berfinsari commented Mar 3, 2019

Refer to #10774 for more info

@berfinsari berfinsari requested a review from a team as a code owner March 3, 2019 05:32
@elasticmachine
Copy link
Copy Markdown
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@sayden sayden added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 4, 2019
@sayden
Copy link
Copy Markdown
Contributor

sayden commented Mar 4, 2019

jenkins, test this please

Copy link
Copy Markdown
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

Overall looks good, just one single change is needed to merge PR. Everything else in comments are minor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you rebase to master, you can remove this line

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you rebase to master, you can use m.Logger().Error(err) here instead

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you rebase to master, you can use m.Logger().Error(err) here instead

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again, this is the only critical change needed here. Check my comments here to avoid a flaky test

@berfinsari berfinsari force-pushed the migration/mb/reportv2/ceph/osd_tree branch from 388b053 to 65c4dbb Compare March 10, 2019 17:10
@berfinsari
Copy link
Copy Markdown
Contributor Author

Thanks for your review. I believe I've addressed all the feedback now.

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Mar 13, 2019

jenkins, test this

Copy link
Copy Markdown
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

Good work!

@sayden sayden merged commit de13893 into elastic:master Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Metricbeat Metricbeat Team:Integrations Label for the Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants