Handle deletion events from watch and purge deleted entries. #62
Conversation
| constexpr const int kMetadataApiDefaultPort = 8000; | ||
| constexpr const char kMetadataApiDefaultResourceTypeSeparator[] = "."; | ||
| constexpr const int kMetadataReporterDefaultIntervalSeconds = 60; | ||
| constexpr const int kMetadataReporterDefaultPurgeDeleted = false; |
There was a problem hiding this comment.
Should we be defaulting to true?
There was a problem hiding this comment.
I'd rather put this through a few more tests. It can be overridden in the config map if necessary.
| }; | ||
|
|
||
| if (container_status) { | ||
| if (container_status && container_status->Has("containerID")) { |
There was a problem hiding this comment.
Why was this added? Do we directly depend on this property anywhere?
There was a problem hiding this comment.
Yes, right below. The deletion events apparently had a status without a container id.
| std::unique_lock<std::mutex> watch_completion(completion_mutex); | ||
| Watcher watcher(callback, std::move(watch_completion), | ||
| config_.VerboseLogging()); | ||
| Watcher watcher(std::bind(&EventCallback, callback, std::placeholders::_1), |
There was a problem hiding this comment.
std::placeholders::_N leaves me a bit puzzled. Is there a way of doing these methods without using unbound arguments?
There was a problem hiding this comment.
They confuse me too. Unfortunately, there is no better way.
If it helps, think about std::bind as: you have to list all of the arguments to the bound function, but some of those can represent the arguments to the function being called -- that's what placeholders are (std::placeholders::_1 is the first argument to the resulting function).
igorpeshansky
left a comment
igorpeshansky
left a comment
There was a problem hiding this comment.
Thanks for the review. PTAL.
| constexpr const int kMetadataApiDefaultPort = 8000; | ||
| constexpr const char kMetadataApiDefaultResourceTypeSeparator[] = "."; | ||
| constexpr const int kMetadataReporterDefaultIntervalSeconds = 60; | ||
| constexpr const int kMetadataReporterDefaultPurgeDeleted = false; |
There was a problem hiding this comment.
I'd rather put this through a few more tests. It can be overridden in the config map if necessary.
| }; | ||
|
|
||
| if (container_status) { | ||
| if (container_status && container_status->Has("containerID")) { |
There was a problem hiding this comment.
Yes, right below. The deletion events apparently had a status without a container id.
| std::unique_lock<std::mutex> watch_completion(completion_mutex); | ||
| Watcher watcher(callback, std::move(watch_completion), | ||
| config_.VerboseLogging()); | ||
| Watcher watcher(std::bind(&EventCallback, callback, std::placeholders::_1), |
There was a problem hiding this comment.
They confuse me too. Unfortunately, there is no better way.
If it helps, think about std::bind as: you have to list all of the arguments to the bound function, but some of those can represent the arguments to the function being called -- that's what placeholders are (std::placeholders::_1 is the first argument to the resulting function).
igorpeshansky
force-pushed
the
igorp-kubernetes-deletion-events
branch
from
4d3eadd to
9c46711
Compare
|
Rebased off |
3 participants
Also a minor refactoring of the watch code.