Huaisi Xu has posted comments on this change. Change subject: IMPALA-3499: Split catalog update ......................................................................
Patch Set 14: (12 comments) http://gerrit.cloudera.org:8080/#/c/3067/14//COMMIT_MSG Commit Message: PS14, Line 9: deserialized > serialized byte array, no? Oh that should be deserializing... PS14, Line 10: > Instead of Done PS14, Line 11: update > the update Done PS14, Line 12: 2GB > 500MB? this is actually TBD. what do you think the limit should be? http://gerrit.cloudera.org:8080/#/c/3067/14/be/src/service/frontend.h File be/src/service/frontend.h: PS14, Line 39: contains > nit: contain I think after "each" I should use xxx+"s"? http://gerrit.cloudera.org:8080/#/c/3067/14/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS14, Line 196: CATALOG_CACHE_UPDATE_BATCH_SIZE_BYTES > call this: Done PS14, Line 1247: incrementalRequest > C++-style naming will do... I just thought that if we keep them exactly the same, it is easier for reader to know that "b" comes from "a". http://gerrit.cloudera.org:8080/#/c/3067/14/be/src/util/jni-util.h File be/src/util/jni-util.h: Line 275: jobjectArray jArrayOfByteArray = > C++-style naming, please. ok. but for this place I think this is a java object though.. I will http://gerrit.cloudera.org:8080/#/c/3067/14/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java: Line 158: TUpdateCatalogCacheRequest merged_update_request = new TUpdateCatalogCacheRequest(); > Needs a comment about what is going on in this method. will do PS14, Line 164: if (!merged_update_request.isSetIs_delta()) { > is_delta is a required field. Can't you just OR- the is_delta fields from e I am not sure why "required field" matters here.. could you elaborate? merged_update_request comes from "new TUpdateCatalogCacheRequest()" which does not set anything? Line 167: if (!incrementalRequest.getCatalog_service_id().equals(default_catalog_service_id)) { > long line Done PS14, Line 171: if (!merged_update_request.isSetUpdated_objects()) { > all of these fields are required, which means they'll be initialised by the No I cannot. that is get by TUpdateCatalogCacheRequest merged_update_request = new TUpdateCatalogCacheRequest();. and this constructor will set them to null. -- To view, visit http://gerrit.cloudera.org:8080/3067 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I176db25124a32944f2396ce8aafbed49cac95928 Gerrit-PatchSet: 14 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-HasComments: Yes
