Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 )
Change subject: [utility] auto-detection of cloud VM instance type ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/14866/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14866/2//COMMIT_MSG@9 PS2, Line 9: an Nit: a http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h File src/kudu/util/cloud/instance_detector.h: http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h@66 PS2, Line 66: std::condition_variable cv_; : std::mutex cv_m_; Could you use our built-in synchronization primitives? Then you can use MonoDelta for timeouts plus have some better instrumentation in DEBUG builds. http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h@68 PS2, Line 68: std::vector<std::thread> detector_threads_; For production code use kudu::Thread which will be included in metrics and the web UI. http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h File src/kudu/util/cloud/instance_metadata.h: PS1: > It's a bit unclear how this is going to be used effectively. At startup, we Continuing the conversation, are you going to attempt to persist the result of cloud detection to avoid the delay with every startup? http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc File src/kudu/util/cloud/instance_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@86 PS1, Line 86: uest_timeout() co > OK, if we want to have that for peace of mind, I can do that. IMHO, it's h Personally I don't mind the hardcoding. These addresses have been defined by the cloud vendors for years and like Alexey said, if they were to change, it's very likely that the rest of the logic in this file will have to change too (i.e. just changing the value of the IP address via gflag won't be enough). -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Sat, 18 Jan 2020 00:38:25 +0000 Gerrit-HasComments: Yes
