Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Add support for watching and ingesting more Kubernetes resources.#194

Merged
supriyagarg merged 5 commits intoStackdriver:v1beta3-working-branchfrom
supriyagarg:generic-watch
Sep 28, 2018
Merged

Add support for watching and ingesting more Kubernetes resources.#194
supriyagarg merged 5 commits intoStackdriver:v1beta3-working-branchfrom
supriyagarg:generic-watch

Conversation

@supriyagarg
Copy link
Copy Markdown
Contributor

The threads to watch resources other than Node and Pod are started based on the Kubernetes resource kind and API version provided in a static vector. This works for all resource types that use the generic callback.

The threads to watch resources other than Node and Pod are started based
on the Kubernetes resource kind and API version provided in a static
vector. This works for all resource types that use the generic callback.
Copy link
Copy Markdown
Contributor

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

Some comments.

Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.h Outdated
Copy link
Copy Markdown
Contributor

@bmoyles0117 bmoyles0117 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

A few comments on the new code.

Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.h Outdated
Comment thread src/kubernetes.h Outdated
Comment thread src/kubernetes.h Outdated
Comment thread src/kubernetes.h Outdated
Comment thread src/kubernetes.cc
Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.h Outdated
Comment thread src/kubernetes.h Outdated
Comment thread src/kubernetes.h Outdated
Copy link
Copy Markdown
Contributor Author

@supriyagarg supriyagarg left a comment

Choose a reason for hiding this comment

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

thanks for the comments Igor - responded.

Comment thread src/kubernetes.cc Outdated
Comment thread src/kubernetes.h Outdated
Copy link
Copy Markdown
Contributor

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

One remaining comment.

Comment thread src/kubernetes.cc Outdated
for (const auto& watch_id: ClusterLevelObjectTypes()) {
const std::string& plural_kind = watch_id.first;
const std::string& api_version = watch_id.second;
object_watch_threads_.emplace(
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.

Should fit on "one line" as well:

        object_watch_threads_.emplace(watch_id, std::thread([=]() {
          reader_.WatchObjects(plural_kind, api_version, cb);
        }));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread src/kubernetes.cc
};
node_watch_thread_ = std::thread([=]() {
object_watch_threads_.emplace(WatchId("nodes", "v1"), std::thread([=]() {
reader_.WatchNodes(watched_node, cb);
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.

Just realized that it's probably more efficient to be capturing reader_ by reference everywhere. But we'll do that as a separate update, as it's orthogonal to this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ack.

Copy link
Copy Markdown
Contributor

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@supriyagarg supriyagarg merged commit 36dab63 into Stackdriver:v1beta3-working-branch Sep 28, 2018
@supriyagarg supriyagarg deleted the generic-watch branch September 28, 2018 19:11
supriyagarg added a commit that referenced this pull request Sep 28, 2018
The threads to watch resources other than Node and Pod are started based
on the Kubernetes resource kind and API version provided in a static
vector. This works for all resource types that use the generic callback.
supriyagarg added a commit that referenced this pull request Sep 29, 2018
The threads to watch resources other than Node and Pod are started based
on the Kubernetes resource kind and API version provided in a static
vector. This works for all resource types that use the generic callback.
bmoyles0117 pushed a commit that referenced this pull request Nov 29, 2018
The threads to watch resources other than Node and Pod are started based
on the Kubernetes resource kind and API version provided in a static
vector. This works for all resource types that use the generic callback.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants