Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13294 )
Change subject: [util] Introduce yaml-cpp to read config files ...................................................................... Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/13294/5/cmake_modules/FindYaml.cmake File cmake_modules/FindYaml.cmake: http://gerrit.cloudera.org:8080/#/c/13294/5/cmake_modules/FindYaml.cmake@22 PS5, Line 22: find_library(YAML_STATIC_LIB libyaml-cpp.a > yaml-cpp does not support build both shared and static libraries concurrent Kudu's default linkage is "automatic", which means dynamic linked binaries for DEBUG/FASTDEBUG/TSAN builds, and static for RELEASE/ASAN builds. So most devs will use dynamic linkage quite a bit, primarily because it speeds up build times and reduces binary sizes. If yaml-cpp can't build both static and dynamic libraries in one go, could we build it twice? See build_gmock() in build-definitions.sh for an example. This would be fine as long the build time of yaml-cpp isn't too bad. http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader.h File src/kudu/util/yamlreader.h: http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader.h@39 PS5, Line 39: // Wraps the YAML parsing functionality of YAML::Node. > Done Could you also doc the public methods? See jsonreader.h for an example. http://gerrit.cloudera.org:8080/#/c/13294/7/src/kudu/util/yamlreader.cc File src/kudu/util/yamlreader.cc: http://gerrit.cloudera.org:8080/#/c/13294/7/src/kudu/util/yamlreader.cc@73 PS7, Line 73: return Status::NotFound(Substitute("parse field $0 error: $1", field, e.what())); Does yaml-cpp offer finer grained errors or exception types that we can map to different Status codes? -- To view, visit http://gerrit.cloudera.org:8080/13294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a Gerrit-Change-Number: 13294 Gerrit-PatchSet: 7 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 14 May 2019 21:11:39 +0000 Gerrit-HasComments: Yes
