Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3499: Batch update catalog cache update. ......................................................................
Patch Set 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/3067/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS8, Line 124: A single object will not be limited by this. How can you guarantee this? Line 1271: } I believe we should log it if L1265 does not return OK. PS8, Line 1306: true How do you know that this is not part of an initial catalog update? PS8, Line 1323: We : // need to do this before updating the catalog. This comment indicates that the modifications above may not be correct. I feel we may not be understanding completely this code path. http://gerrit.cloudera.org:8080/#/c/3067/8/be/src/service/impala-server.h File be/src/service/impala-server.h: PS8, Line 743: locks for all UpdateCatalogCache(), necessary for split update since other threads may You need to mention what this lock protects (see example in L747). PS8, Line 745: catalogUpdate_lock private fields should end with a '_', also the name should be catalog_update_lock_ (we are not in Java :)) http://gerrit.cloudera.org:8080/#/c/3067/8/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: PS8, Line 631: // Sent from the impalad BE to FE with the results of each CatalogUpdate heartbeat. : // Contains details on all catalog objects that need to be updated. I don't think the comment is accurate given that the result of a CatalogUpdate heartbeat may be applied in batches. Can you update it? PS8, Line 638: frontend can set is_ready accordingly I don't remember what 'is_ready' is in the FE and I don't think I have to :). Also, the comment talks about 'last update' although there is no context around it (see my previous comment). Please update the comment. http://gerrit.cloudera.org:8080/#/c/3067/8/fe/src/main/java/com/cloudera/impala/catalog/ImpaladCatalog.java File fe/src/main/java/com/cloudera/impala/catalog/ImpaladCatalog.java: PS8, Line 145: lastSyncedCatalogVersion_ = newCatalogVersion; : // Cleanup old entries in the log. : catalogDeltaLog_.garbageCollect(lastSyncedCatalogVersion_); Since the newCatalogVersion is updated from the last object (CATALOG), wouldn't it make sense to set lastSyncedCatalogVersion and garbage collect old entries in the log after we process the last update? PS8, Line 150: // Notify all the threads waiting on a catalog update. : synchronized (catalogUpdateEventNotifier_) { : catalogUpdateEventNotifier_.notifyAll(); : } Are you sure it is ok to notify waiting threads even though you are in the middle of an update? -- 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: 8 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
