Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17410 )
Change subject: A poc Oat++ Rest server ...................................................................... Patch Set 1: (9 comments) Just took a quick look, adding mostly high-level comments http://gerrit.cloudera.org:8080/#/c/17410/1//COMMIT_MSG Commit Message: PS1: How do you see using this functionality in the Kudu codebase? I guess conceptually it might be some sort of a wrapper for oat++ to use in Kudu code, similar like WebServer class used in server_base.{h,cc}. http://gerrit.cloudera.org:8080/#/c/17410/1//COMMIT_MSG@7 PS1, Line 7: A poc Oat++ Rest server I think it makes sense to separate this at least into two patches: 1) patch to introduce oat++ and oatswagger into Kudu's thirdparty 2) a wrapper for oat/swagger to use in Kudu codebase http://gerrit.cloudera.org:8080/#/c/17410/1/src/kudu/rest/Readme.adoc File src/kudu/rest/Readme.adoc: PS1: If that's an example, should http://gerrit.cloudera.org:8080/#/c/17410/1/src/kudu/rest/src/AppComponent.h File src/kudu/rest/src/AppComponent.h: PS1: Here and for all other files: drop the 'src' element of the path, it's already under src/kudu http://gerrit.cloudera.org:8080/#/c/17410/1/src/kudu/rest/src/AppComponent.h@18 PS1, Line 18: #ifndef APPCOMPONENT_H : #define APPCOMPONENT_H Consider using '#pragma once' instead http://gerrit.cloudera.org:8080/#/c/17410/1/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/17410/1/thirdparty/build-definitions.sh@1087 PS1, Line 1087: #Oatpp nit: how much this comment adds to the 'build_oatpp()' function name? If not much, then remove it maybe? http://gerrit.cloudera.org:8080/#/c/17410/1/thirdparty/build-definitions.sh@1094 PS1, Line 1094: -DCMAKE_BUILD_TYPE=release \ : -DCMAKE_INSTALL_PREFIX=$PREFIX \ : -DCMAKE_SHARED_LIBS=Off \ : $OATPP_SOURCE nit: the indent is off -- see how those commands are formatted elsewhere in this file. http://gerrit.cloudera.org:8080/#/c/17410/1/thirdparty/build-definitions.sh@1098 PS1, Line 1098: -j Use -j$PARALLEL because in some build scenarios we want to control the level of build parallelism. http://gerrit.cloudera.org:8080/#/c/17410/1/thirdparty/build-definitions.sh@1111 PS1, Line 1111: make -j install : : popd nit: the indent is off -- To view, visit http://gerrit.cloudera.org:8080/17410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id76453453f982ffed6ec3e0b126fc1583295dfa5 Gerrit-Change-Number: 17410 Gerrit-PatchSet: 1 Gerrit-Owner: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 10 May 2021 19:13:05 +0000 Gerrit-HasComments: Yes
