Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20868 )
Change subject: KUDU-3543 Fix Content-Type headers ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/20868/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20868/3//COMMIT_MSG@10 PS3, Line 10: The problem is that in the webserver's path handler functions we only : use a bool called 'is_styled' to decide on the style mode [1]. Probably, it makes sense to explain why this is/was a problem? Was/Is something working not as expected? If yes, please describe the essence of the issue. http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/master/master-test.cc@3818 PS3, Line 3818: std:: nit for here and below: remove the namespace prefix since all the types are already in the 'using' directive in the beginning of the file http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/master/master-test.cc@3822 PS3, Line 3822: for (const auto& endpoint_pair : GetCommonWebserverEndpoints()) { : endpoint_type_map.insert(endpoint_pair); : } : for (const auto& endpoint_pair : GetMasterWebserverEndpoints(table_id)) { : endpoint_type_map.insert(endpoint_pair); : } nit: in C++-17 there is unordered_map::merge() method that could be handy to get rid of for() cycles http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/master/master-test.cc@3836 PS3, Line 3836: LOG(INFO) << Substitute("Checking endpoint '/$0' for Content-Type header", endpoint); How does this help with automated testing? If it doesn't help in any way, please remove this. http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/master/master_path_handlers.cc@876 PS3, Line 876: constexpr const StyleMode style_mode = StyleMode::STYLED; While you at this, consider removing this constant and use StyleMode::STYLED directly in this method. Otherwise, starting line 902 it becomes a bit confusing. http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/server/default_path_handlers.cc@444 PS3, Line 444: StyleMode style_mode = StyleMode::STYLED; nit: add 'const' or even 'constexpr'? Alternatively, drop this variable and use StyleMode::STYLED directly in call sites? http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/server/webserver.h File src/kudu/server/webserver.h: http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/server/webserver.h@70 PS3, Line 70: stlye_mode style_mode http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/server/webserver.h@122 PS3, Line 122: content header This seems to be a part of unfinished sentence? http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/tserver/tablet_server-test.cc@628 PS3, Line 628: std:: nit for here and below: drop the std:: prefix http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/tserver/tablet_server-test.cc@643 PS3, Line 643: LOG(INFO) << Substitute("Checking tablet server endpoint '/$0' for Content-Type header", : endpoint); How is this going to help in automated testing? http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/util/test_util.h File src/kudu/util/test_util.h: http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/util/test_util.h@206 PS3, Line 206: inline std::unordered_map<std::string, std::string> GetCommonWebserverEndpoints() { Why to insist on in-lining this and other functions below? Could they be implemented in test-util.cc? http://gerrit.cloudera.org:8080/#/c/20868/3/src/kudu/util/test_util.h@207 PS3, Line 207: "text/html" nit: would it make sense to have this and few other content type strings as constants and use them as such when defining the contents of those maps, instead of the string literals? -- To view, visit http://gerrit.cloudera.org:8080/20868 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I746dcfbaadb2fb95292c2d4047cb7adb9971b42f Gerrit-Change-Number: 20868 Gerrit-PatchSet: 3 Gerrit-Owner: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Jan 2024 07:52:09 +0000 Gerrit-HasComments: Yes
