You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/12/07 01:18:59 UTC
[kudu-CR] WIP [util] utilities to get info on cloud instance
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14866
Change subject: WIP [util] utilities to get info on cloud instance
......................................................................
WIP [util] utilities to get info on cloud instance
A utility class to collect information on the type of the instance
in public clouds.
Follow-up changelists will make use of this functionality
while running the auto-detection of the type of the cloud
instance upon startup of kudu-master and kudu-tserver.
WIP:
* collect feedback on the placement of the functionality
(libkudu_util)
* usage of libcurl as a first-class dependency (it was
builg/test-only for some period of time)
Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cloud/instance_metadata.cc
A src/kudu/util/cloud/instance_metadata.h
3 files changed, 318 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/1
--
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: newchange
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] WIP [util] utilities to get info on cloud instance
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar 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:
(4 comments)
I'm assuming there isn't a generic open-source library available to fetch such information, yet?
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
Nit: Suffix If for interface
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?
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@60
PS1, Line 60: MonoDelta InstanceMetadataBase::request_timeout() const {
: static const MonoDelta kTimeout = MonoDelta::FromMilliseconds(500);
: return kTimeout;
: }
Why not return const reference to member variable kTimeout_ or a copy?
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?
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 07 Dec 2019 02:51:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14866
to look at the new patch set (#3).
Change subject: [utility] auto-detection of cloud VM instance type
......................................................................
[utility] auto-detection of cloud VM instance type
Added a utility to auto-detect the type of VM instance for
AWS, Azure and GCE public clouds.
Follow-up changelists will make use of this functionality if
auto-configuring the built-in NTP client upon startup of kudu-master
and kudu-tserver.
Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cloud/instance_detector-test.cc
A src/kudu/util/cloud/instance_detector.cc
A src/kudu/util/cloud/instance_detector.h
A src/kudu/util/cloud/instance_metadata.cc
A src/kudu/util/cloud/instance_metadata.h
6 files changed, 657 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/3
--
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: newpatchset
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14866
to look at the new patch set (#7).
Change subject: [utility] auto-detection of cloud VM instance type
......................................................................
[utility] auto-detection of cloud VM instance type
Added a utility to auto-detect the type of VM instance for
AWS, Azure and GCE public clouds.
Follow-up changelists will make use of this functionality if
auto-configuring the built-in NTP client upon startup of kudu-master
and kudu-tserver.
Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cloud/instance_detector-test.cc
A src/kudu/util/cloud/instance_detector.cc
A src/kudu/util/cloud/instance_detector.h
A src/kudu/util/cloud/instance_metadata.cc
A src/kudu/util/cloud/instance_metadata.h
6 files changed, 719 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/7
--
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: newpatchset
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] WIP [util] utilities to get info on cloud instance
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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:
(1 comment)
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@60
PS1, Line 60: MonoDelta InstanceMetadataBase::request_timeout() const {
: static const MonoDelta kTimeout = MonoDelta::FromMilliseconds(500);
: return kTimeout;
: }
> Why not return const reference to member variable kTimeout_ or a copy?
Why returning const reference is any better here than kTimeout_ as a copy? MonoDelta is a 64-bit object.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Dec 2019 20:43:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14866
to look at the new patch set (#5).
Change subject: [utility] auto-detection of cloud VM instance type
......................................................................
[utility] auto-detection of cloud VM instance type
Added a utility to auto-detect the type of VM instance for
AWS, Azure and GCE public clouds.
Follow-up changelists will make use of this functionality if
auto-configuring the built-in NTP client upon startup of kudu-master
and kudu-tserver.
Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cloud/instance_detector-test.cc
A src/kudu/util/cloud/instance_detector.cc
A src/kudu/util/cloud/instance_detector.h
A src/kudu/util/cloud/instance_metadata.cc
A src/kudu/util/cloud/instance_metadata.h
6 files changed, 719 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/5
--
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: newpatchset
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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: "169.254.169.123"
> Thank you for the feedback. It seems some extra clarification is needed he
All right, I added gflags to be able to control these.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 20:35:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector-test.cc
File src/kudu/util/cloud/instance_detector-test.cc:
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector-test.cc@83
PS4, Line 83:
> To reduce possibility of flaky tests, can we go lower than this, FromNanose
A few microsecond interval seems to be low enough because the interval covers spawning detector threads as well, and I ran it many 1K times in different configurations, not seeing any flakes. But I agree that MonoDelta::FromNanoseconds(1) is a better choice here.
Done.
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h
File src/kudu/util/cloud/instance_detector.h:
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@38
PS4, Line 38: s run b
> running-> run
Done
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@56
PS4, Line 56: n didn't reco
> auto-detection
Done
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@60
PS4, Line 60: ut was
> typo
Done
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@70
PS4, Line 70: e specified inst
> I can't find this field in this change. Perhaps it was renamed.
Good catch: this has changed, indeed. I updated it.
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.cc
File src/kudu/util/cloud/instance_detector.cc:
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.cc@84
PS4, Line 84: us wakeups are ignored by the
> An instance can't be running on multiple cloud vendors, so the first one to
Done
--
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: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 01:43:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14866 )
Change subject: [utility] auto-detection of cloud VM instance type
......................................................................
[utility] auto-detection of cloud VM instance type
Added a utility to auto-detect the type of VM instance for
AWS, Azure and GCE public clouds.
Follow-up changelists will make use of this functionality if
auto-configuring the built-in NTP client upon startup of kudu-master
and kudu-tserver.
Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Reviewed-on: http://gerrit.cloudera.org:8080/14866
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cloud/instance_detector-test.cc
A src/kudu/util/cloud/instance_detector.cc
A src/kudu/util/cloud/instance_detector.h
A src/kudu/util/cloud/instance_metadata.cc
A src/kudu/util/cloud/instance_metadata.h
6 files changed, 719 insertions(+), 13 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14866
to look at the new patch set (#4).
Change subject: [utility] auto-detection of cloud VM instance type
......................................................................
[utility] auto-detection of cloud VM instance type
Added a utility to auto-detect the type of VM instance for
AWS, Azure and GCE public clouds.
Follow-up changelists will make use of this functionality if
auto-configuring the built-in NTP client upon startup of kudu-master
and kudu-tserver.
Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cloud/instance_detector-test.cc
A src/kudu/util/cloud/instance_detector.cc
A src/kudu/util/cloud/instance_detector.h
A src/kudu/util/cloud/instance_metadata.cc
A src/kudu/util/cloud/instance_metadata.h
6 files changed, 708 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/4
--
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: newpatchset
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14866/5/src/kudu/util/cloud/instance_metadata.cc
File src/kudu/util/cloud/instance_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/14866/5/src/kudu/util/cloud/instance_metadata.cc@175
PS5, Line 175: don't
doesn't
--
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: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 17:31:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 19:19:45 +0000
Gerrit-HasComments: No
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 2:
> (4 comments)
>
> I'm assuming there isn't a generic open-source library available to
> fetch such information, yet?
I didn't find any particular that would fit. There are many things like https://github.com/dgzlopes/cloud-detect , https://github.com/banzaicloud/cloudinfo , but that's not what's needed in this context.
Please let me know if you find anything that seems appropriate.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 23:27:10 +0000
Gerrit-HasComments: No
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14866/5/src/kudu/util/cloud/instance_metadata.cc
File src/kudu/util/cloud/instance_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/14866/5/src/kudu/util/cloud/instance_metadata.cc@175
PS5, Line 175: don't
> doesn't
Whoops.
Done.
--
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: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 18:09:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14866
to look at the new patch set (#6).
Change subject: [utility] auto-detection of cloud VM instance type
......................................................................
[utility] auto-detection of cloud VM instance type
Added a utility to auto-detect the type of VM instance for
AWS, Azure and GCE public clouds.
Follow-up changelists will make use of this functionality if
auto-configuring the built-in NTP client upon startup of kudu-master
and kudu-tserver.
Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cloud/instance_detector-test.cc
A src/kudu/util/cloud/instance_detector.cc
A src/kudu/util/cloud/instance_detector.h
A src/kudu/util/cloud/instance_metadata.cc
A src/kudu/util/cloud/instance_metadata.h
6 files changed, 719 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/6
--
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: newpatchset
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 18 Jan 2020 00:38:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 21:32:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] utilities to get info on cloud instance
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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:
(1 comment)
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: "169.254.169.123"
> Sorry, I never saw IP address hardcoded. IMHO, it should be placed in textu
We could place some configuration properties into a configuration file, but kudu-tserver and kudu-master don't have a configuration files as of now except, maybe, flagsfile. Probably, it would make sense to make this IP address and other properties configurable via run-time flags, but I don't think these are going to change ever till cloud providers support dedicated NTP servers. If the API used here is changed so it requires something to change in these would-be-configuration-properties, I expect _all_ this code should be rewritten. At least that's the impression I got while reading public documentation on the these topics.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 21:20:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 7: Code-Review+2
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.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jan 2020 18:12:56 +0000
Gerrit-HasComments: No
[kudu-CR] WIP [util] utilities to get info on cloud instance
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev 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:
(1 comment)
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: "169.254.169.123"
> We could place some configuration properties into a configuration file, but
I agree with Vlad here. IMHO, IPs and URLs that are not in our control should not be hardcoded. A run-time flag with default value seems reasonable. We wouldn't want to be making an emergency release when any of these IPs or URLs change.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 20:31:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] utilities to get info on cloud instance
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin 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//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14866/1//COMMIT_MSG@20
PS1, Line 20: builg
Mistyped "build"
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: "169.254.169.123"
This kind of information should be stored in some configuration file.
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@104
PS1, Line 104: "http://169.254.169.254"
: "/latest/meta-data/instance-id"
This kind of information should be stored in some configuration file.
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@121
PS1, Line 121: "http://169.254.169.254"
: "/metadata/instance/compute/vmId?api-version=2018-10-01&format=text"
This kind of information should be stored in some configuration file.
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.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2020 01:27:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 )
Change subject: [utility] auto-detection of cloud VM instance type
......................................................................
Patch Set 4: Code-Review+1
(6 comments)
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector-test.cc
File src/kudu/util/cloud/instance_detector-test.cc:
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector-test.cc@83
PS4, Line 83: FromMicroseconds
To reduce possibility of flaky tests, can we go lower than this, FromNanoseconds()?
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h
File src/kudu/util/cloud/instance_detector.h:
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@38
PS4, Line 38: running
running-> run
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@56
PS4, Line 56: autodetection
auto-detection
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@60
PS4, Line 60: cloude
typo
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.h@70
PS4, Line 70: result_metadata_
I can't find this field in this change. Perhaps it was renamed.
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.cc
File src/kudu/util/cloud/instance_detector.cc:
http://gerrit.cloudera.org:8080/#/c/14866/4/src/kudu/util/cloud/instance_detector.cc@84
PS4, Line 84: result_detector_idx_ == kNoIdx
An instance can't be running on multiple cloud vendors, so the first one to update result_detector_idx_ wins the race and breaks the loop in case of spurious wake ups for other threads?
Might be worth adding a comment.
--
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: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 22:31:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(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: it
> Nit: a
Done
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:
:
> Could you use our built-in synchronization primitives? Then you can use Mon
Done
http://gerrit.cloudera.org:8080/#/c/14866/2/src/kudu/util/cloud/instance_detector.h@68
PS2, Line 68:
> For production code use kudu::Thread which will be included in metrics and
Done
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h
File src/kudu/util/cloud/instance_metadata.h:
PS1:
> Continuing the conversation, are you going to attempt to persist the result
Probably, but I think that's better to done in a separate changelist.
From the other hand, an extra second on start of kudu-{master,tserver} is nothing compared with hours loading the on-disk data structures and bootstrapping tablets. In addition, it would be necessary to provide an extra flag/tool to clean the persisted information. So, I'm still weighing pros and contras.
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: "169.254.169.123"
> Personally I don't mind the hardcoding. These addresses have been defined b
Thank you for the feedback. It seems some extra clarification is needed here: I'll re-read the docs making sure there is a clear contract for keeping these parameters constant.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 18 Jan 2020 05:04:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] utilities to get info on cloud instance
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 04:34:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.h
File src/kudu/util/cloud/instance_detector.h:
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.h@79
PS3, Line 79: std::vector<scoped_refptr<Thread>> threads_;
: std::vector<std::unique_ptr<InstanceMetadata>> metadata_;
Could combine into one vector with a struct.
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.cc
File src/kudu/util/cloud/instance_detector.cc:
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.cc@62
PS3, Line 62: {
I think you could use a loop here, perhaps with a vector<pair<unique_ptr<InstanceMetadata>, string>>?
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h
File src/kudu/util/cloud/instance_metadata.h:
PS1:
> Probably, but I think that's better to done in a separate changelist.
That's true; I'm mostly worried about tests that do a fair amount of daemon restarting. Maybe we don't have very many of those though.
--
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: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 18 Jan 2020 05:47:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.h
File src/kudu/util/cloud/instance_detector.h:
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.h@79
PS3, Line 79: struct DetectorInfo {
: std::unique_ptr<InstanceMetadata> metadata;
> Could combine into one vector with a struct.
Done
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.cc
File src/kudu/util/cloud/instance_detector.cc:
http://gerrit.cloudera.org:8080/#/c/14866/3/src/kudu/util/cloud/instance_detector.cc@62
PS3, Line 62: const auto deadline = MonoTime::Now() + timeout_;
> I think you could use a loop here, perhaps with a vector<pair<unique_ptr<In
Done
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.h
File src/kudu/util/cloud/instance_metadata.h:
PS1:
> That's true; I'm mostly worried about tests that do a fair amount of daemon
With a follow-up changelist, this feature is used by the external-mini-cluster test and by the external_mini_cluster-test-test, so not many tests use it now.
--
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: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 20:34:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] utilities to get info on cloud instance
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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:
(3 comments)
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: "169.254.169.123"
> This kind of information should be stored in some configuration file.
I'm not sure I follow. Configuration file for what? This is a pre-defined IP address for NTP server at any AWS instance.
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@104
PS1, Line 104: "http://169.254.169.254"
: "/latest/meta-data/instance-id"
> This kind of information should be stored in some configuration file.
ditto
http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@121
PS1, Line 121: "http://169.254.169.254"
: "/metadata/instance/compute/vmId?api-version=2018-10-01&format=text"
> This kind of information should be stored in some configuration file.
Ditto -- configuration file for what?
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2020 06:02:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [utility] auto-detection of cloud VM instance type
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Bankim Bhavsar, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14866
to look at the new patch set (#2).
Change subject: [utility] auto-detection of cloud VM instance type
......................................................................
[utility] auto-detection of cloud VM instance type
Added an utility to auto-detect the type of VM instance for
AWS, Azure and GCE public clouds.
Follow-up changelists will make use of this functionality if
auto-configuring the built-in NTP client upon startup of kudu-master
and kudu-tserver.
Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cloud/instance_detector-test.cc
A src/kudu/util/cloud/instance_detector.cc
A src/kudu/util/cloud/instance_detector.h
A src/kudu/util/cloud/instance_metadata.cc
A src/kudu/util/cloud/instance_metadata.h
6 files changed, 626 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/14866/2
--
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: newpatchset
Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b
Gerrit-Change-Number: 14866
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] WIP [util] utilities to get info on cloud instance
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin 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:
(1 comment)
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: "169.254.169.123"
> I'm not sure I follow. Configuration file for what? This is a pre-defined
Sorry, I never saw IP address hardcoded. IMHO, it should be placed in textual configuration file.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Jan 2020 05:52:31 +0000
Gerrit-HasComments: Yes