Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-3499: Split catalog update ......................................................................
Patch Set 13: (9 comments) http://gerrit.cloudera.org:8080/#/c/3067/13//COMMIT_MSG Commit Message: PS13, Line 6: : IMPALA-3499: Split catalog update : : Java does not support byte array larger than 2GB. This : patch splits update into a list of updates that are all : less than 2GB. Can you please be a little verbose here ? Based on the commit message its not clear that we are only splitting the catalog update from Catalog->Impalad updates. I think it helps if you can summarize the behavior "before" and "after" this patch. http://gerrit.cloudera.org:8080/#/c/3067/13/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS13, Line 196: CATALOG_CACHE_UPDATE_BATCH_SIZE May be rename it to CATALOG_CACHE_UPDATE_BATCH_SIZE_BYTES or something? (I'm assuming its in bytes) PS13, Line 1262: if (catalog_object.type == TCatalogObjectType::CATALOG) { : incrementalRequest->__set_catalog_service_id( : catalog_object.catalog.catalog_service_id); : new_catalog_version = catalog_object.catalog_version; : } Shouldn't this be moved after L1282? Basically after updating incrementalRequest pointer incase the length exceeds? Else, we might end up calling __set_catalog_service_id() and updated_objects() on different incrementalRequest objects if condition in L1278 succeeds. Line 1280: incrementalRequest = &update_reqs.back(); You are not setting the "required" field "is_delta". Does it work without it? PS13, Line 1278: f (current_topic_update_size + len > CATALOG_CACHE_UPDATE_BATCH_SIZE) { : update_reqs.push_back(TUpdateCatalogCacheRequest()); : incrementalRequest = &update_reqs.back(); : current_topic_update_size = 0; : } Better to move this to post L1254 as soon as we have the len(), then it addresses the above comment too. What do you think? http://gerrit.cloudera.org:8080/#/c/3067/13/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java: PS13, Line 158: req may be name it mergedReq or something similar? PS13, Line 164: incrementalRequest.isIs_delta() Like I mentioned in frontend.cc, it doesn't look like you are setting this for all incrementalRequest objects, can you please double check? PS13, Line 165: if (!incrementalRequest.getCatalog_service_id().equals(default_catalog_service_id) Why this logic? Why don't we set if its the default? PS13, Line 171: getRemoved_objects getUpdated_objects() -- 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: 13 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
