Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15292 )
Change subject: [webserver] add information on time source ...................................................................... Patch Set 2: (14 comments) http://gerrit.cloudera.org:8080/#/c/15292/2//COMMIT_MSG Commit Message: PS2: > I implemented storing/retrieving of the effective settings for time sourc I think those extra knobs _could_ make sense in the general case, but specifically here we're talking about /config, and I think having /config be a reflection of gflag state is a good thing, since it's, well, /config (i.e. "configuration"). http://gerrit.cloudera.org:8080/#/c/15292/2//COMMIT_MSG@9 PS2, Line 9: This patch adds the following information into the '/config' endpoint Would love to see a screenshot or two showing the new page. http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@293 PS2, Line 293: static void FillSecurityConfigs(EasyJson* output) { Apart from the kFoo consolidation, has this changed at all? http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@344 PS2, Line 344: static const char* const kName = "name"; : static const char* const kValue = "value"; : static const char* const kId = "id"; : static const char* const kComment = "comment"; These need to be the same as in FillSecurityConfigs() for the mustache stuff to work, right? Maybe make them global and share? http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@373 PS2, Line 373: best the best http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@375 PS2, Line 375: In case of other settings Nit: Otherwise, http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@375 PS2, Line 375: setting Nit: drop this http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@376 PS2, Line 376: always Nit: drop this. http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@387 PS2, Line 387: Servers Nit: NTP Servers http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@392 PS2, Line 392: In case Nit: don't need http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@392 PS2, Line 392: the --builtin_ntp_servers flag Nit: just --builtin_ntp_servers, don't need 'the' or 'flag'. http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@395 PS2, Line 395: hosting the hosting http://gerrit.cloudera.org:8080/#/c/15292/2/src/kudu/server/default_path_handlers.cc@396 PS2, Line 396: the --builtin_ntp_servers flag Nit: don't need 'the' or 'flag'. http://gerrit.cloudera.org:8080/#/c/15292/2/www/config.mustache File www/config.mustache: http://gerrit.cloudera.org:8080/#/c/15292/2/www/config.mustache@44 PS2, Line 44: {{/security_configs}} This seems out of place? -- To view, visit http://gerrit.cloudera.org:8080/15292 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ff07808a1a534f3546e88ff13bc876b98afdb7e Gerrit-Change-Number: 15292 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Wed, 26 Feb 2020 01:48:26 +0000 Gerrit-HasComments: Yes
