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 7: > Having reviewed the code again, I noticed that none of the instance > metadata subclasses actually need the expressivity that inheritance > offers. All the overridden methods are just pass-through "return > this flag", and the only one that deviates slightly (AWS's "get NTP > server" method) can be modeled as a passthrough "return nullptr". > > What do you think of combining all of the classes into something > more monolithic? I'm thinking: > - A "registry" of sorts (vector of structs) that declaratively > describes the properties of each cloud provider. I'm tempted to say > it's static, but if that prevents gflag overrides from working > properly, it can be constructed on the fly with each instantiation > of the instance detector (since that's a rare operation). > - That registry would be consumed at runtime by the instance > detector, which would create a detection thread for each entry in > the registry. > - The detection thread would monolithically include the curl logic > (because at this point there's no advantage to the separation). > > That should drastically reduce the amount of code/comments and make > everything easier to follow, at the cost of lack of flexibility if > down the line we do need the full expressivity of inheritance to > control per-cloud behavior. > > Leaving you a +2 in case you'd rather keep the current approach. > Having reviewed the code again, I noticed that none of the instance > metadata subclasses actually need the expressivity that inheritance > offers. All the overridden methods are just pass-through "return > this flag", and the only one that deviates slightly (AWS's "get NTP > server" method) can be modeled as a passthrough "return nullptr". > > What do you think of combining all of the classes into something > more monolithic? I'm thinking: > - A "registry" of sorts (vector of structs) that declaratively > describes the properties of each cloud provider. I'm tempted to say > it's static, but if that prevents gflag overrides from working > properly, it can be constructed on the fly with each instantiation > of the instance detector (since that's a rare operation). > - That registry would be consumed at runtime by the instance > detector, which would create a detection thread for each entry in > the registry. > - The detection thread would monolithically include the curl logic > (because at this point there's no advantage to the separation). > > That should drastically reduce the amount of code/comments and make > everything easier to follow, at the cost of lack of flexibility if > down the line we do need the full expressivity of inheritance to > control per-cloud behavior. > > Leaving you a +2 in case you'd rather keep the current approach. The original idea was to have library methods to work with a cloud instance's metadata, where the IP address/FQDN of the dedicated NTP server is just one of many other properties. I think the suggested alternative approach looks like a good fit for the purpose of just detecting the type of the cloud instance, and that's exactly what we use it for now :) I think I can go ahead and implement the suggested alternative approach, but make it a separate follow-up patch. -- 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: 7 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: Wed, 22 Jan 2020 19:19:45 +0000 Gerrit-HasComments: No
