You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2019/05/09 07:29:03 UTC

[kudu-CR] [yaml] Introduce yaml-cpp

Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13294


Change subject: [yaml] Introduce yaml-cpp
......................................................................

[yaml] Introduce yaml-cpp

KUDU-1948 notice that yaml is better than json when use it as a config
file.
yaml-cpp [https://github.com/jbeder/yaml-cpp] is the most popular C++
yaml library I can find on GitHub.
This patch introduce yaml-cpp into Kudu, do some simple wrap, and add
some unit tests.

Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
---
M CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
6 files changed, 49 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/13294/1
-- 
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: newchange
Gerrit-Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
Gerrit-Change-Number: 13294
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>

[kudu-CR] [yaml] Introduce yaml-cpp

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13294

to look at the new patch set (#2).

Change subject: [yaml] Introduce yaml-cpp
......................................................................

[yaml] Introduce yaml-cpp

KUDU-1948 notice that yaml is better than json when use it as a config
file.
yaml-cpp [https://github.com/jbeder/yaml-cpp] is the most popular C++
yaml library I can find on GitHub.
This patch introduce yaml-cpp into Kudu, do some simple wrap, and add
some unit tests.

Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
---
M CMakeLists.txt
A cmake_modules/FindYaml.cmake
M src/kudu/util/CMakeLists.txt
A src/kudu/util/yamlreader-test.cc
A src/kudu/util/yamlreader.cc
A src/kudu/util/yamlreader.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
10 files changed, 463 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/13294/2
-- 
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: newpatchset
Gerrit-Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
Gerrit-Change-Number: 13294
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [util] Introduce yaml-cpp to read config files

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13294 )

Change subject: [util] Introduce yaml-cpp to read config files
......................................................................

[util] Introduce yaml-cpp to read config files

KUDU-1948 notice that yaml is better than json when use it as a config
file.
yaml-cpp [https://github.com/jbeder/yaml-cpp] is the most popular C++
yaml library I can find on GitHub.
This patch introduce yaml-cpp into Kudu, do some simple wrap, and add
some unit tests.

Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
Reviewed-on: http://gerrit.cloudera.org:8080/13294
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M CMakeLists.txt
A cmake_modules/FindYaml.cmake
M src/kudu/util/CMakeLists.txt
A src/kudu/util/yamlreader-test.cc
A src/kudu/util/yamlreader.cc
A src/kudu/util/yamlreader.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
10 files changed, 495 insertions(+), 1 deletion(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
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: merged
Gerrit-Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
Gerrit-Change-Number: 13294
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [util] Introduce yaml-cpp to read config files

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

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 <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 14 May 2019 21:11:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] [util] Introduce yaml-cpp to read config files

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13294 )

Change subject: [util] Introduce yaml-cpp to read config files
......................................................................


Patch Set 5:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc
File src/kudu/util/yamlreader-test.cc:

http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc@44
PS5, Line 44:  
> Nit: space here
Done


http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc@45
PS5, Line 45: Env::Default()
> KuduTest has an env_ you can use.
Done


http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc@66
PS5, Line 66:   ASSERT_TRUE(r->ExtractScalar(r->node(), "foo", &val).IsCorruption());
> The Extract functions are static, so shouldn't we call them as such? i.e. Y
Done


http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc@75
PS5, Line 75:   ASSERT_TRUE(r->ExtractScalar(r->node(), "foo", &val).IsCorruption());
> Shouldn't this return something IsNotFound since 'foo' doesn't exist in the
Done


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: class YamlReader {
> Please doc the class and its public methods so people know how to use it.
Done


http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader.h@77
PS5, Line 77:                      const std::string& field,
            :                      T* result) {
> Indentation.
Done


http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh@928
PS5, Line 928:   CFLAGS="$EXTRA_CFLAGS -fPIC" \
             :     CXXFLAGS="$EXTRA_CXXFLAGS -fPIC" \
> Why do we need to explicitly add -fPIC?
Done


http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh@932
PS5, Line 932:     cmake \
> Pass in EXTRA_CMAKE_FLAGS too.
Done


http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh@939
PS5, Line 939:   make -j$PARALLEL $EXTRA_MAKEFLAGS install
> Use ${NINJA:-make} here to prefer ninja if it exists.
Done


http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/vars.sh
File thirdparty/vars.sh:

http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/vars.sh@239
PS5, Line 239: yaml-cpp-yaml-cpp-
> Why is yaml-cpp listed twice? Can we remove one?
No, because the original downloaded package name is like: yaml-cpp-yaml-cpp-0.6.2.tar.gz



-- 
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: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 14 May 2019 10:10:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] [util] Introduce yaml-cpp to read config files

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13294

to look at the new patch set (#5).

Change subject: [util] Introduce yaml-cpp to read config files
......................................................................

[util] Introduce yaml-cpp to read config files

KUDU-1948 notice that yaml is better than json when use it as a config
file.
yaml-cpp [https://github.com/jbeder/yaml-cpp] is the most popular C++
yaml library I can find on GitHub.
This patch introduce yaml-cpp into Kudu, do some simple wrap, and add
some unit tests.

Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
---
M CMakeLists.txt
A cmake_modules/FindYaml.cmake
M src/kudu/util/CMakeLists.txt
A src/kudu/util/yamlreader-test.cc
A src/kudu/util/yamlreader.cc
A src/kudu/util/yamlreader.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
10 files changed, 463 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/13294/5
-- 
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: newpatchset
Gerrit-Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
Gerrit-Change-Number: 13294
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [util] Introduce yaml-cpp to read config files

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Yingchun Lai 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:

(2 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
> Could we also generate a shared library version of yaml-cpp to use for dyna
yaml-cpp does not support build both shared and static libraries concurrently, and I'm confused when to use dynamically link in kudu?


http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh@928
PS5, Line 928:   CFLAGS="$EXTRA_CFLAGS -fPIC" \
             :     CXXFLAGS="$EXTRA_CXXFLAGS -fPIC" \
> Done
Link error occured if not. Errors like:
03:16:50 [ 30%] Linking CXX shared library ../../../lib/libkudu_util.so
03:16:50 /usr/bin/ld: ../../../../../thirdparty/installed/uninstrumented/lib/libyaml-cpp.a(exceptions.cpp.o): relocation R_X86_64_32S against `_ZTVN4YAML9ExceptionE' can not be used when making a shared object; recompile with -fPIC
03:16:50 ../../../../../thirdparty/installed/uninstrumented/lib/libyaml-cpp.a: error adding symbols: Bad value



-- 
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 <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 14 May 2019 11:10:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] [util] Introduce yaml-cpp to read config files

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13294

to look at the new patch set (#9).

Change subject: [util] Introduce yaml-cpp to read config files
......................................................................

[util] Introduce yaml-cpp to read config files

KUDU-1948 notice that yaml is better than json when use it as a config
file.
yaml-cpp [https://github.com/jbeder/yaml-cpp] is the most popular C++
yaml library I can find on GitHub.
This patch introduce yaml-cpp into Kudu, do some simple wrap, and add
some unit tests.

Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
---
M CMakeLists.txt
A cmake_modules/FindYaml.cmake
M src/kudu/util/CMakeLists.txt
A src/kudu/util/yamlreader-test.cc
A src/kudu/util/yamlreader.cc
A src/kudu/util/yamlreader.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
10 files changed, 495 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/13294/9
-- 
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: newpatchset
Gerrit-Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
Gerrit-Change-Number: 13294
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [util] Introduce yaml-cpp to read config files

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13294

to look at the new patch set (#7).

Change subject: [util] Introduce yaml-cpp to read config files
......................................................................

[util] Introduce yaml-cpp to read config files

KUDU-1948 notice that yaml is better than json when use it as a config
file.
yaml-cpp [https://github.com/jbeder/yaml-cpp] is the most popular C++
yaml library I can find on GitHub.
This patch introduce yaml-cpp into Kudu, do some simple wrap, and add
some unit tests.

Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
---
M CMakeLists.txt
A cmake_modules/FindYaml.cmake
M src/kudu/util/CMakeLists.txt
A src/kudu/util/yamlreader-test.cc
A src/kudu/util/yamlreader.cc
A src/kudu/util/yamlreader.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
10 files changed, 475 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/13294/7
-- 
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: newpatchset
Gerrit-Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a
Gerrit-Change-Number: 13294
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>