Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata ...................................................................... Patch Set 6: (14 comments) I'm pretty happy with this change, probably good to get another pair of eyes. Let's ask Dimitris to review. http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-server.h File be/src/catalog/catalog-server.h: http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-server.h@78 PS6, Line 78: void AddPendingTopicItem(std::string key, const uint8_t* topic_data, uint32_t size, topic_data -> item_data (the topic is a container of items, and we are passing the data of a single item) http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util-test.cc File be/src/catalog/catalog-util-test.cc: http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util-test.cc@40 PS6, Line 40: for (int i = 0; i < 2000 * 1024 * 1024; ++i) { // almost 2GB * reserve() the size * why almost 2GB? is there any significance to 2GB? add comment http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h File be/src/catalog/catalog-util.h: http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@36 PS6, Line 36: public: space before private/protected/public http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@45 PS6, Line 45: /// will be skipped and the catalog update will still proceed. will be skipped and the next valid object is returned. http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@46 PS6, Line 46: virtual jobject next(JNIEnv* env) = 0; Better to add a virtual destructor http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@54 PS6, Line 54: static jclass pair_class; nit: use common abbreviations for JNI stuff, "cl" for "class" and "ctor" for "constructor", i.e.: pair_cl, pair_ctor, boolean_cl, boolean_ctor http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.cc File be/src/catalog/catalog-util.cc: http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.cc@106 PS6, Line 106: pos_ += 1; ++pos http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.cc@118 PS6, Line 118: remove newline http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc File be/src/service/fe-support.cc: http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@428 PS6, Line 428: JNIEXPORT void JNICALL I'm thinking this function should return a bool for success/failure. That way the caller can decide whether what to do with this issue. I think we should probably log something and move on in most cases. http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@435 PS6, Line 435: // Throw any exception into the frontend Comment seems wrong, the FE won't see an exception. http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@455 PS6, Line 455: JNIEXPORT void JNICALL Same comment about returning bool. http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@468 PS6, Line 468: JNIEXPORT void JNICALL Same comment about returning bool. http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/util/jni-util.h File be/src/util/jni-util.h: http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/util/jni-util.h@172 PS6, Line 172: public: single space before public/private http://gerrit.cloudera.org:8080/#/c/8825/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/8825/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@404 PS6, Line 404: { move to prev line -- To view, visit http://gerrit.cloudera.org:8080/8825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae Gerrit-Change-Number: 8825 Gerrit-PatchSet: 6 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Comment-Date: Wed, 24 Jan 2018 21:56:14 +0000 Gerrit-HasComments: Yes
