Huaisi Xu has posted comments on this change. Change subject: IMPALA-3499: Batch update catalog cache update. ......................................................................
Patch Set 5: (6 comments) > do your changes to ImpaladCatalog prevent publishing incremental updates? I think we do not need to do that since we do not have consistency guarantee in the first place(without this patch). Sorry just saw this.. http://gerrit.cloudera.org:8080/#/c/3067/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 1281: len = item.value.size(); > try to avoid reusing variables for different purposes like this. In this ca I think this is the same purpose? DeserializeThriftMsg needs this? Line 1299: update_size + len > what if one object is larger than the max? How about testing update_size > I think we must let it pass otherwise impalad will have inconsistence metadata as what statestore thinks? I think the max size is unintentionally guaranteed by catalogd's limitation? Line 1307: UpdateCatalogCache > Is it possible that the state exposed by the catalog can now be inconsisten This can happen at any time without this patch as well. this call does not synchronized across the whole jvm? I think this change does not change any consistency guarantee? Line 1312: } > Oh is that what line 1355 is for? It's a bit confusing for the logic to be I think the logic can flow like that? and less code? what do you think I can do anything.. :) Line 1354: if (update_status.ok()) { : update_status = exec_env_->frontend()->UpdateCatalogCache(update_req, &resp); : } > catalog_update_lock isn't guaranteed to be held here if topic_entries.size( if the lock is not acquired before this then there is no split update happened? Line 1357: if (catalog_update_lock.owns_lock()) catalog_update_lock.unlock(); > Is this necessary, or will catalogUpdate_lock be unlocked automatically whe That will, but this is faster and more explicit? I can remove this if you like? -- 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: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu <[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-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
