Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 )
Change subject: [utility] auto-detection of cloud VM instance type ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h File src/kudu/util/cloud/instance_metadata.h: http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h@35 PS1, Line 35: InstanceMetadata > Personally I don't think there's much to be gained by separating interface +1 to Adar's point. http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h@55 PS1, Line 55: InvalidState > You mean IllegalState? Whoops. Yes, IllegalState. http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h@108 PS1, Line 108: avaiable > available Done 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@40 PS1, Line 40: 500 > Alexey and I talked about this offline: the idea is for the Init() function Yes, as Adar explained, that's the plan. The idea is to spawn multiple detectors at once and collect the results: the first successful response before timing out each and every detector should give us the result. We strive to reduce the startup times as much as possible. I added corresponding comment. http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@86 PS1, Line 86: "169.254.169.123" > I agree with Vlad here. IMHO, IPs and URLs that are not in our control shou OK, if we want to have that for peace of mind, I can do that. IMHO, it's highly unlikely these are going to change without the need to rewrite the whole implementation of retrieving the information from cloud metadata servers. http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@92 PS1, Line 92: std:: > std:: not needed in this .cc file here and other places, right? Done http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@109 PS1, Line 109: Status AzureInstanceMetadata::GetNtpServer(string* server) const { > warning: parameter 'server' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@142 PS1, Line 142: "http://metadata.google.internal"; : "/computeMetadata/v1/instance/id" > This kind of information should be stored in some configuration file. I added more docs on each class, so hopefully it's clearer these properties are not going to change ever. -- 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: 1 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Greg Solovyev <gsolov...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin <verjov...@cloudera.com> Gerrit-Comment-Date: Fri, 17 Jan 2020 21:32:19 +0000 Gerrit-HasComments: Yes
