[kudu-CR] [clock] change clock source selection for 'auto'
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17582 ) Change subject: [clock] change clock source selection for 'auto' .. [clock] change clock source selection for 'auto' This patch changes the logic to select particular time source when the pseudo-source 'auto' used for --time_source. Before this patch, the 'system' time source would be auto-selected if a Kudu server with --time_source=auto is run in an environment where the instance detector isn't aware of a dedicated NTP server (those are usually available via the host-only interface, at least for EC2 and GCE instances). After this patch, the 'builtin' time source would be auto-selected if a Kudu server runs with --time_source=auto in environment where the instance detector isn't aware of dedicated NTP servers AND the --builtin_ntp_servers flag is set to a valid value. Otherwise, if --builtin_ntp_servers flag is set to an empty/invalid value, 'system' becomes the auto-selected time source for platforms supporting get_ntptime() API, otherwise the catch-all case is used to auto-select 'system_unsync' as the time source. I also added a new test scenario to cover the updated functionality. Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Reviewed-on: http://gerrit.cloudera.org:8080/17582 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M src/kudu/clock/builtin_ntp.cc M src/kudu/clock/builtin_ntp.h M src/kudu/clock/hybrid_clock-test.cc M src/kudu/clock/hybrid_clock.cc M src/kudu/clock/hybrid_clock.h M src/kudu/clock/ntp-test.cc M src/kudu/server/default_path_handlers.cc 7 files changed, 146 insertions(+), 60 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [clock] change clock source selection for 'auto'
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17582 ) Change subject: [clock] change clock source selection for 'auto' .. Patch Set 3: > Patch Set 3: Code-Review+2 Thank you for review, Andrew! -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Jul 2021 18:29:48 + Gerrit-HasComments: No
[kudu-CR] [clock] change clock source selection for 'auto'
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17582 ) Change subject: [clock] change clock source selection for 'auto' .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Jul 2021 18:22:54 + Gerrit-HasComments: No
[kudu-CR] [clock] change clock source selection for 'auto'
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17582 ) Change subject: [clock] change clock source selection for 'auto' .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc@476 PS3, Line 476: // Switch to the built-in NTP client unless the set of reference servers > One of the nice things about the current iteration of this is that the sele Yep, this iteration is deterministic in the terms of current configuration and doesn't depend much on network and other runtime issues. The expectation is that the built-in client has less strict requirements to synchronize because it's not needed to sync the system clock (assuming the set of the NTP servers is the same for ntpd/chronyd and the built-in client). With that, it might happen that the built-in NTP client is able to synchronize quite fast, but the system clock cannot synchronized for a long time even if NTP server is latched with the reference NTP servers (e.g., the ntpd is running with '-x' option and the original offset of the local time was several seconds). -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 09 Jul 2021 22:59:49 + Gerrit-HasComments: Yes
[kudu-CR] [clock] change clock source selection for 'auto'
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17582 ) Change subject: [clock] change clock source selection for 'auto' .. Patch Set 3: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc@476 PS3, Line 476: // Switch to the built-in NTP client unless the set of reference servers > Sure: it's possible to detect that, but it will require extra time waiting One of the nice things about the current iteration of this is that the selected time source is entirely dependent on the configured environment, regardless of any networking woes. Depending on what we want "auto" to be, that might be good enough. If it is meant to pick the best clock for the current environment and current configs, we seem to already be doing the right thing. If the goal is to reduce the number of times there are instances where an unsynchronized clock can crash a server, I suppose that's another story, and that would bring into question the robustness of the built-in clock vs the system clock for initial synchronization. What are our expectations of the builtin's ability to synchronize vs typical "system" deployments? With default deployments, would we expect that if the system client can't synchronize, the built-in client would also have issues synchronizing? -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 06 Jul 2021 22:09:25 + Gerrit-HasComments: Yes
[kudu-CR] [clock] change clock source selection for 'auto'
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17582 ) Change subject: [clock] change clock source selection for 'auto' .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc@476 PS3, Line 476: // Switch to the built-in NTP client unless the set of reference servers > In the case the built-in NTP client can't reach the configured ntp servers, Sure: it's possible to detect that, but it will require extra time waiting for the built-in client to sync-up and then bail if it could not. The risk here is that due to transient networking issue some nodes might fallback to the system, while others might be able to get it working with the built-in NTP client. With that, do you think we need some sort of flag to gate that probing behavior? -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 21 Jun 2021 19:20:09 + Gerrit-HasComments: Yes
[kudu-CR] [clock] change clock source selection for 'auto'
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/17582 ) Change subject: [clock] change clock source selection for 'auto' .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc@476 PS3, Line 476: // Switch to the built-in NTP client unless the set of reference servers In the case the built-in NTP client can't reach the configured ntp servers, should we continue falling back to the system ntp? Is that something we can easily detect? -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 21 Jun 2021 19:15:04 + Gerrit-HasComments: Yes
[kudu-CR] [clock] change clock source selection for 'auto'
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17582 ) Change subject: [clock] change clock source selection for 'auto' .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/17582/3/src/kudu/clock/hybrid_clock.cc@260 PS3, Line 260: uint64_t now_latest = GetPhysicalValueMicros(now) + error; > warning: The right operand of '+' is a garbage value [clang-analyzer-core.U This is a garbage warning: NowWithErrorOrDie() cannot return not setting the value for 'error'. Ignored. -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 12 Jun 2021 00:27:09 + Gerrit-HasComments: Yes
[kudu-CR] [clock] change clock source selection for 'auto'
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17582 ) Change subject: [clock] change clock source selection for 'auto' .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/17582/2/src/kudu/clock/hybrid_clock.h File src/kudu/clock/hybrid_clock.h: http://gerrit.cloudera.org:8080/#/c/17582/2/src/kudu/clock/hybrid_clock.h@206 PS2, Line 206: // The 'builtin_ntp_servers' is used in case of TimeSource::BUILTIN_NTP_SYNC. > this line is no longer needed. Done http://gerrit.cloudera.org:8080/#/c/17582/2/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/17582/2/src/kudu/clock/hybrid_clock.cc@260 PS2, Line 260: uint64_t now_latest = GetPhysicalValueMicros(now) + error; > warning: The right operand of '+' is a garbage value [clang-analyzer-core.U This is a garbage warning: NowWithErrorOrDie() cannot return not setting the value for 'error'. Ignored. -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 11 Jun 2021 22:52:14 + Gerrit-HasComments: Yes
[kudu-CR] [clock] change clock source selection for 'auto'
Hello Tidy Bot, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17582 to look at the new patch set (#3). Change subject: [clock] change clock source selection for 'auto' .. [clock] change clock source selection for 'auto' This patch changes the logic to select particular time source when the pseudo-source 'auto' used for --time_source. Before this patch, the 'system' time source would be auto-selected if a Kudu server with --time_source=auto is run in an environment where the instance detector isn't aware of a dedicated NTP server (those are usually available via the host-only interface, at least for EC2 and GCE instances). After this patch, the 'builtin' time source would be auto-selected if a Kudu server runs with --time_source=auto in environment where the instance detector isn't aware of dedicated NTP servers AND the --builtin_ntp_servers flag is set to a valid value. Otherwise, if --builtin_ntp_servers flag is set to an empty/invalid value, 'system' becomes the auto-selected time source for platforms supporting get_ntptime() API, otherwise the catch-all case is used to auto-select 'system_unsync' as the time source. I also added a new test scenario to cover the updated functionality. Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 --- M src/kudu/clock/builtin_ntp.cc M src/kudu/clock/builtin_ntp.h M src/kudu/clock/hybrid_clock-test.cc M src/kudu/clock/hybrid_clock.cc M src/kudu/clock/hybrid_clock.h M src/kudu/clock/ntp-test.cc M src/kudu/server/default_path_handlers.cc 7 files changed, 146 insertions(+), 60 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/17582/3 -- To view, visit http://gerrit.cloudera.org:8080/17582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I72cb54c488b2aa2c73acd6e6e5f6e50dd5811175 Gerrit-Change-Number: 17582 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
