Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 )
Change subject: WIP [util] utilities to get info on cloud instance ...................................................................... Patch Set 1: (5 comments) 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 don't know which (if any) cloud provider we're on, right? So I guess we'll be instantiating an InstanceMetadataBase, then cycling through the various cloud options until we find the one we're on? To expedite that, shouldn't we try to detect all cloud providers in parallel? Doesn't seem like the code as-written does that. BTW, another idea is to write the detected cloud provider into some file. Maybe the existing block_manager instance files, or the FS instance files. Then we only have to run the cloud detection once, when we first create the on-disk filesystem. And maybe we can add a flag to rerun detection at startup for folks who want to migrate their files (or maybe not). What do you think? http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h@35 PS1, Line 35: InstanceMetadata > Nit: Suffix If for interface Personally I don't think there's much to be gained by separating interface from implementation. It's not like in Java where the language sort of forces you to do that, and I think having fewer levels in the class hierarchy makes the code easier to follow. http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h@55 PS1, Line 55: InvalidState You mean IllegalState? http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h@108 PS1, Line 108: avaiable available Below too. 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 > That looks pretty low to me, is this expected across all cloud providers? Alexey and I talked about this offline: the idea is for the Init() function to be called synchronously at server startup, so that we can use cloud detection to reconfigure certain gflag default values. As such, we want the timeout to be just high enough to work effectively, but as low as possible to avoid slowing down non-cloud deployments too much. It's unclear whether that will be tenable. Another option is to kick off asynchronous cloud detection as early in server boot as possible, so that by the time we need it (i.e. setting up the HybridClock), hopefully it's done and we don't have to block at all. -- 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: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin <verjov...@cloudera.com> Gerrit-Comment-Date: Mon, 09 Dec 2019 04:34:37 +0000 Gerrit-HasComments: Yes
