Wrap pod callback for pod and container metadata with a try/catch. #77
Conversation
bmoyles0117
requested review from
dhrupadb,
igorpeshansky,
qingling128 and
supriyagarg
| callback(std::move(result_vector)); | ||
| } catch (const json::Exception& e) { | ||
| LOG(ERROR) << e.what(); | ||
| } |
There was a problem hiding this comment.
A couple of questions:
- Why is the error not being caught + logged by the try-catch block around the Watchmaster (L1077)?
- For parity, please add a try-catch block in the NodeCallback too
There was a problem hiding this comment.
+1. This should have been logged in WatchMaster. I think the issue here is that the thread dies, but the right fix is to restart the watch, not to swallow the error.
| @@ -1049,9 +1049,13 @@ void KubernetesReader::PodCallback( | |||
| MetadataUpdater::UpdateCallback callback, | |||
| const json::Object* pod, Timestamp collected_at, bool is_deleted) const | |||
| throw(json::Exception) { | |||
There was a problem hiding this comment.
update the method signature - since the exception is being caught and logged, the throw(..) can be dropped.
| std::vector<MetadataUpdater::ResourceMetadata> result_vector = | ||
| GetPodAndContainerMetadata(pod, collected_at, is_deleted); | ||
| callback(std::move(result_vector)); | ||
| } catch (const json::Exception& e) { |
There was a problem hiding this comment.
I don't think this is the right fix for the situation -- this will simply ignore a JSON error. The assumption here is that if we got an invalid hunk of JSON, the rest of the hunks will also be garbage, so we shouldn't bother with the current connection -- it's better to re-establish the watch.
| callback(std::move(result_vector)); | ||
| } catch (const json::Exception& e) { | ||
| LOG(ERROR) << e.what(); | ||
| } |
There was a problem hiding this comment.
+1. This should have been logged in WatchMaster. I think the issue here is that the thread dies, but the right fix is to restart the watch, not to swallow the error.
|
Superseded by #79. |
3 participants
No description provided.