Don't update owners cache until the value is available. #160
Conversation
davidbtucker
left a comment
davidbtucker
left a comment
There was a problem hiding this comment.
Do you want to add a test for this?
|
Can't add a test, because it requires being able to mock out a permission error from the API. Do we have that functionality already? |
|
I think once my pending PRs are in, we could add a way for the mock k8s master to return a permission error. Feel free to assign an issue to me to do this. |
src/kubernetes.cc
Outdated
| ") disappeared"); | ||
| } | ||
| } catch (const QueryException& e) { | ||
| owners_.erase(encoded_ref); // Remove the nullptr entry from owners_. |
There was a problem hiding this comment.
Instead of erasing the reference on an exception, can we just not add it until we get a success?
There was a problem hiding this comment.
The way it's implemented now, the lookup in line 936 will construct the entry if it's not present. As I understood it, that's the recommended way to do it in C++ 11. Without that approach, there's no clean way to have a single return from the method.
Tried switching to a lookup followed by an emplace, as you suggested. PTAL.
igorpeshansky
changed the title
src/kubernetes.cc
Outdated
| json::value& owner = found.first->second; | ||
| if (found.second) { // Not found, inserted new. | ||
| auto found = owners_.find(encoded_ref); | ||
| if (found == owners_.end()) { // Not found, add new. |
There was a problem hiding this comment.
If the owner is found, let's return the owner right away.
There was a problem hiding this comment.
Tackled this another way by going back to a single return. PTAL.
3 participants
Fixes a segfault in
GetOwner.