Add the support for kafka in cAdvisor's storage, including output of container id and labels #1033
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
|
I signed it! |
|
CLAs look good, thanks! |
| } | ||
|
|
||
| // Container reference contains enough information to uniquely identify a container | ||
| type ContainerReference struct { | ||
| // The container id | ||
| Id string `json:"id"` |
There was a problem hiding this comment.
Why is this required for a reference, but optional for the spec? I think these should be consistent, or at least the Reference should have weaker requirements than the spec.
|
could you please re-order the import packages as the follow order:
|
|
Did you run |
|
@k8s-bot ok to test |
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
|
@timstclair I've addressed your PR feedback. |
|
Travis-CI is not liking the Kafka.go header going from 2015 -> 2016 |
|
Oh yeah, that needs to be fixed. |
|
It looks like you somehow picked up a bunch of extraneous commits. Can you try rebasing? |
|
@timstclair I tried rebasing and it this is what I got 👎 I can close this PR and resubmit as a fresh one that is cleanly in one commit if thats ok. |
|
It looks like your commits for this PR are interspersed with merged commits, in which case you might need to manually repair it. I'd suggest trying |
|
CLAs look good, thanks! |
|
@timstclair I was able to clean it up. |
|
Great, but unfortunately github won't show the diff now because of all the godeps changes. Could you please move the godeps changes to a separate commit, so that I can review the other files separately? Thanks! |
|
@timstclair I've separated them. |
|
@timstclair Is there anything else you need me to do? |
|
@timstclair should close this and resubmit a clean PR? Thanks |
|
Sorry, I was out of office last week. I'll take a final look at this later today. |
|
Just a few more comments. Thanks for your patience. |
|
@timstclair comments addressed. Thanks |
|
LGTM, just 1 small comment |
…container id and labels Addressing PR Feedback Addressing PR Feedback
|
@timstclair comment addressed. Thanks |
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
|
LGTM. Thanks! |
|
@k8s-bot ok to test |
Add the support for kafka in cAdvisor's storage, including output of container id and labels
Steps to run and configure with custom broker and topic. Default broker is
localhost:9092. Default topic isstats.sudo ./cadvisor -storage_driver=kafka -storage_driver_kafka_broker_list=localhost:9092 -storage_driver_kafka_topic=statsThis will push json encoded container statistics onto the designated topic. Includes additional output of container id and labels, added to the v1 container spec.