Huaisi Xu 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 n will do 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' will do 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 incrementalRe I think it does not matter. the logic in frontend guarantees that we are good as long as we have this in any incrementalRequest. we can also push empty incrementalRequest. frontend can handle those. Does this answer your concern? Line 1280: incrementalRequest = &update_reqs.back(); > You are not setting the "required" field "is_delta". Does it work without i Yes. the default value is 0. I just left it unset since we will get exactly one such request among all of those. 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 add see above? 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? ok. PS13, Line 164: incrementalRequest.isIs_delta() > Like I mentioned in frontend.cc, it doesn't look like you are setting this the is_delta field will be set in the first incrementalRequest guaranteed. so we just set it once here. ? 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? because we set the catalog_service_id only in exactly one incrementalRequest. the other will be default. PS13, Line 171: getRemoved_objects > getUpdated_objects() oh this is bad!! thank you! -- 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
