Memoize owner objects instead of querying the master all the time. #46
Conversation
igorpeshansky
requested review from
bmoyles0117,
qingling128 and
supriyagarg
src/kubernetes.cc
Outdated
| const auto path_component = KindPath(api_version, kind); | ||
|
|
||
| const std::string encoded_ref = boost::algorithm::join( | ||
| std::vector<std::string>{api_version, kind, ns, name}, "/"); |
There was a problem hiding this comment.
No can do, sorry -- namespace is a C++ keyword. :-)
|
LGTM |
igorpeshansky
force-pushed
the
igorp-kubernetes-refactoring
branch
from
02e756f to
38d3db0
Compare
igorpeshansky
force-pushed
the
igorp-kubernetes-memoize-owners
branch
from
bad0e2d to
94f1019
Compare
igorpeshansky
changed the base branch from
igorp-kubernetes-refactoring
to
master
|
There's an important question here that needs to be answered. Do we ever intend to use anything other than the |
| return QueryMaster(path_component.first + "/namespaces/" + ns + "/" + | ||
| path_component.second + "/" + name); | ||
| owner = std::move( | ||
| QueryMaster(path_component.first + "/namespaces/" + ns + "/" + |
There was a problem hiding this comment.
Is it possible that this throws an exception, without killing the thread, which would end up leaving owner null, which would end up throwing another exception the next time due to owner->Clone() being on a nullptr? If so, can we add a sanity check of if (owner == nullptr || found.second) {?
There was a problem hiding this comment.
An exception will not allow the rest of this function to continue -- it'll be propagated up the stack, and caught, likely in KubernetesReader::MetadataQuery. So such a check would be superfluous.
src/kubernetes.cc
Outdated
| const auto path_component = KindPath(api_version, kind); | ||
|
|
||
| const std::string encoded_ref = boost::algorithm::join( | ||
| std::vector<std::string>{api_version, kind, ns, name}, "/"); |
There was a problem hiding this comment.
Long story short, it will likely be better to use the owner_ref->Get<json::String>("uid"); instead of name, to ensure that the parent we're caching is directly related to this object. WDYT?
There was a problem hiding this comment.
There's an important question here that needs to be answered. Do we ever intend to use anything other than the
topLevelControllerTypeandtopLevelControllerNamefrom the top level controller? If we don't for now, but may in the future, we should at least leave a comment in big scary letters that this memoizes by parent name, not parent uid. It's possible for someone to delete a deployment, and create a new one with the same name and the metadata agent will get "stuck" on the outdated deployment metadata.
Good point. I've changed the code to use the uid as the cache key.
| return QueryMaster(path_component.first + "/namespaces/" + ns + "/" + | ||
| path_component.second + "/" + name); | ||
| owner = std::move( | ||
| QueryMaster(path_component.first + "/namespaces/" + ns + "/" + |
There was a problem hiding this comment.
An exception will not allow the rest of this function to continue -- it'll be propagated up the stack, and caught, likely in KubernetesReader::MetadataQuery. So such a check would be superfluous.
src/kubernetes.cc
Outdated
| const auto path_component = KindPath(api_version, kind); | ||
|
|
||
| const std::string encoded_ref = boost::algorithm::join( | ||
| std::vector<std::string>{api_version, kind, ns, name}, "/"); |
There was a problem hiding this comment.
There's an important question here that needs to be answered. Do we ever intend to use anything other than the
topLevelControllerTypeandtopLevelControllerNamefrom the top level controller? If we don't for now, but may in the future, we should at least leave a comment in big scary letters that this memoizes by parent name, not parent uid. It's possible for someone to delete a deployment, and create a new one with the same name and the metadata agent will get "stuck" on the outdated deployment metadata.
Good point. I've changed the code to use the uid as the cache key.
| path_component.second + "/" + name)); | ||
| // Sanity check: because we are looking up by name, the object we get | ||
| // back might have a different uid. | ||
| const json::Object* owner_obj = owner->As<json::Object>(); |
There was a problem hiding this comment.
Nit: the naming of the variables seems inconsistent. owner_uid prevents a clash with uid, so perhaps we should have owner_metadata for consistency? Not important to address this.
There was a problem hiding this comment.
Funny you should mention that -- I initially had it as owner_metadata, but that caused some ugly line breaks, so I went with just metadata. I can change it if you insist.
No description provided.