Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/17410 )
Change subject: A poc Oat++ Rest server ...................................................................... Patch Set 1: Code-Review-1 (9 comments) I've taken note of all of the comments, and looking into correcting the mistakes, and make the build definition smaller so all the oat++ modules can be in one place. Also formulate the classes into their respective places in the kudu codebase if possible, but at the current stage, I think it is better to keep them in one place, as this is just example to show that this framework can work in conjunction with kudu. 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 conc Yep, it is pretty much the same as the logic used with Squeasel to create the webserver. Introduce wrapper class for Oat++, where it could be infused with kudu client logic (call it restserver.h, for example), then add it to the server base for the initialization. But the downside to this is it will be a separate server. On the other hand, I'm looking for ways to integrate the wrapped oat++ logic into the existing server related classes(master,tserver) so they could continue serving their purpose along with receiving rest requests(but oat++ essentially needs a server to be started to use its swagger side stuff, so have yet to figure out a way to do this.) 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: Noted, will be done asap. 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 ... be better formed to suit the kudu.adoc style? :) 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 alre Okay 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 Okay 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 n Not much, noted. 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 Okay 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 leve Okay 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 Okay, i'm trying to integrate the build logic of separate modules into one definition as well. -- 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: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 11 May 2021 05:40:49 +0000 Gerrit-HasComments: Yes
