Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15168 )
Change subject: [util] remove timeout parameter for cloud::InstanceDetector ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/15168/1/src/kudu/util/cloud/instance_detector.cc File src/kudu/util/cloud/instance_detector.cc: http://gerrit.cloudera.org:8080/#/c/15168/1/src/kudu/util/cloud/instance_detector.cc@65 PS1, Line 65: // Add an extra delay in addition to the declared timeout for fetching : // instance metadata. The idea is to have the limit on the time to wait : // in case if instance metadata fetcher could not fulfill its promise to : // return valid result or an error within the init_timeout() interval. Yeah but is this actually possible? Or a hypothetical concern? BTW, is it actually safe to WaitUntil? Let's suppose one detector is very slow and we end up timing out on L96. If we return a bad Status, there's a very good chance that the caller to Detect() will pass that Status back up the call stack, destroying objects along the way (or at the end when the server is destroyed). In doing so, they may destroy the InstanceDetector itself even though there's an outstanding detector thread running in GetInstanceInfo. That thread will then write to uninitialized memory. http://gerrit.cloudera.org:8080/#/c/15168/1/src/kudu/util/cloud/instance_metadata.h File src/kudu/util/cloud/instance_metadata.h: http://gerrit.cloudera.org:8080/#/c/15168/1/src/kudu/util/cloud/instance_metadata.h@78 PS1, Line 78: class InstanceMetadataBase : public InstanceMetadata { Any chance you'd be interested in combining this with InstanceMetadata? The interface vs. implementation divide is necessary in Java, but I think it just makes code harder to follow in C++ (unless it's part of a library). -- To view, visit http://gerrit.cloudera.org:8080/15168 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6f7cac9273e884abda22b9e9a7d37fa3e307c2d4 Gerrit-Change-Number: 15168 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Thu, 06 Feb 2020 01:36:34 +0000 Gerrit-HasComments: Yes
