Skip to content

Contrib Extension for Ambari Metrics Emitter - #3767

Merged
b-slim merged 7 commits into
apache:masterfrom
nishantmonu51:ambari-metrics-emitter
Dec 19, 2016
Merged

Contrib Extension for Ambari Metrics Emitter#3767
b-slim merged 7 commits into
apache:masterfrom
nishantmonu51:ambari-metrics-emitter

Conversation

@nishantmonu51

@nishantmonu51 nishantmonu51 commented Dec 9, 2016

Copy link
Copy Markdown
Member

extension to enable druid to send metrics to ambari metrics server
(https://cwiki.apache.org/confluence/display/AMBARI/Metrics)

@nishantmonu51 nishantmonu51 added this to the 0.9.3 milestone Dec 9, 2016
@nishantmonu51

nishantmonu51 commented Dec 9, 2016

Copy link
Copy Markdown
Member Author

@b-slim @himanshug Since it is very similar to graphite emitter. Added you to the assignees. Feel free to reassign.

@fjy

fjy commented Dec 9, 2016

Copy link
Copy Markdown
Contributor

plz update docs include extensions.md

himanshug
himanshug reviewed Dec 12, 2016
View reviewed changes
Comment thread ...bari-metrics-emitter/src/main/java/io/druid/emitter/ambari/metrics/AmbariMetricsEmitter.java Outdated

@himanshug himanshug Dec 12, 2016

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.

this check is redundant

@nishantmonu51 nishantmonu51 Dec 16, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed.

himanshug
himanshug reviewed Dec 12, 2016
View reviewed changes
Comment thread ...bari-metrics-emitter/src/main/java/io/druid/emitter/ambari/metrics/AmbariMetricsEmitter.java Outdated

@himanshug himanshug Dec 12, 2016

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.

isn't this going to report each event individually and not wait for the batch to complete?

@nishantmonu51 nishantmonu51 Dec 16, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

nice catch, fixed.

@nishantmonu51

nishantmonu51 commented Dec 16, 2016

Copy link
Copy Markdown
Member Author

@fjy Added docs. @himanshug: review comments addressed.

@nishantmonu51 nishantmonu51 reopened this Dec 16, 2016
b-slim
b-slim reviewed Dec 16, 2016
View reviewed changes
Comment thread extensions-contrib/ambari-metrics-emitter/pom.xml Outdated

@b-slim b-slim Dec 16, 2016

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.

is this for testing ?

@nishantmonu51 nishantmonu51 Dec 19, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed. removed druid-processing extra dependency.

b-slim
b-slim reviewed Dec 16, 2016
View reviewed changes
Comment thread extensions-contrib/ambari-metrics-emitter/pom.xml Outdated

@b-slim b-slim Dec 16, 2016

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.

do we need this here ?

@nishantmonu51 nishantmonu51 Dec 19, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes. Ambari artifacts are not published in mvn central.

b-slim
b-slim reviewed Dec 16, 2016
View reviewed changes
Comment thread ...bari-metrics-emitter/src/main/java/io/druid/emitter/ambari/metrics/AmbariMetricsEmitter.java Outdated

@b-slim b-slim Dec 16, 2016

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.

can we call it DEFAULT_FLUSH_TIMEOUT_MS

b-slim
b-slim reviewed Dec 16, 2016
View reviewed changes
Comment thread ...bari-metrics-emitter/src/main/java/io/druid/emitter/ambari/metrics/AmbariMetricsEmitter.java Outdated

@b-slim b-slim Dec 16, 2016

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.

this will log it twice the message.

b-slim
b-slim reviewed Dec 16, 2016
View reviewed changes
Comment thread ...bari-metrics-emitter/src/main/java/io/druid/emitter/ambari/metrics/AmbariMetricsEmitter.java Outdated

@b-slim b-slim Dec 16, 2016

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.

need to make sure that only one call to flush is in the queue maybe synchronize around start ?

b-slim
b-slim reviewed Dec 16, 2016
View reviewed changes
Comment thread ...etrics-emitter/src/main/java/io/druid/emitter/ambari/metrics/AmbariMetricsEmitterConfig.java Outdated

@b-slim b-slim Dec 16, 2016

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.

add unit please

b-slim
b-slim reviewed Dec 16, 2016
View reviewed changes
Comment thread ...etrics-emitter/src/main/java/io/druid/emitter/ambari/metrics/AmbariMetricsEmitterConfig.java Outdated

@b-slim b-slim Dec 16, 2016

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.

be not ne

b-slim
b-slim reviewed Dec 16, 2016
View reviewed changes

@b-slim b-slim left a comment

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.

LGTM minor comments.

@himanshug

himanshug commented Dec 16, 2016

Copy link
Copy Markdown
Contributor

👍 from my side, looks pretty close to same as graphite emitter.

@nishantmonu51

nishantmonu51 commented Dec 19, 2016

Copy link
Copy Markdown
Member Author

@b-slim review comments addressed.

@fjy

fjy commented Dec 19, 2016

Copy link
Copy Markdown
Contributor

@b-slim any more comments?

@b-slim

b-slim commented Dec 19, 2016

Copy link
Copy Markdown
Contributor

👍

@b-slim
b-slim merged commit f576a0f into apache:master Dec 19, 2016
dgolitsyn pushed a commit to metamx/druid that referenced this pull request Feb 14, 2017
@nishantmonu51 @dgolitsyn
Contrib Extension for Ambari Metrics Emitter (apache#3767)
580afe6 
* Contrib Extension for Ambari Metrics Emitter

extension to enable druid to send metrics to ambari metrics server
(https://cwiki.apache.org/confluence/display/AMBARI/Metrics)

review comments

switch to public repo

* review comments

* add docs

* fix pom version

* Add link for doc page in extensions.md

* remove unused imports

* review comments

review comments

remove unused dependency

review comment
@gianm gianm mentioned this pull request Feb 28, 2017
Closed
This was referenced Sep 5, 2021
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@himanshug himanshug himanshug left review comments
@b-slim b-slim b-slim left review comments

Assignees

No one assigned

Labels

Feature Release Notes

Projects

None yet

Milestone

0.10.0

Development

Successfully merging this pull request may close these issues.

5 participants

@nishantmonu51 @fjy @himanshug @b-slim @gianm