Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5538: Use explicit catalog versions for deleted objects ......................................................................
Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/7731/5/be/src/catalog/catalog-server.h File be/src/catalog/catalog-server.h: PS5, Line 150: added nit: May be 'added/updated/removed' to be consistent with other places? http://gerrit.cloudera.org:8080/#/c/7731/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS5, Line 1366: ; Could you add 'len' too, given this is trace logging anyway. (might be helpful in determining each key's contribution). PS5, Line 1399: // Nothing to do here. Remove? Kinda seems obvious http://gerrit.cloudera.org:8080/#/c/7731/5/fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java File fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java: PS5, Line 53: identiy nit: typo PS5, Line 123: if (first.getType() != second.getType()) return false; This seems to already be handled by the next line (by generating a different name). http://gerrit.cloudera.org:8080/#/c/7731/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: PS5, Line 318: TGetAllCatalogObjectsResponse resp = new TGetAllCatalogObjectsResponse(); : resp.setUpdated_objects(new ArrayList<TCatalogObject>()); : resp.setDeleted_objects(new ArrayList<TCatalogObject>()); : resp.setMax_catalog_version(Catalog.INITIAL_CATALOG_VERSION); Move all of this into getCatalogChanges() and make it return resp? Line 350: * to 'resp'. May be worth adding that this method expects that the global read lock is held for consistency. Line 369: if (tbl.getCatalogVersion() > fromVersion) { Given we hold the global readLock() already (which means that the version cannot change while we are inside this function), can we move this above L366 ? This helps avoiding wait on un-necessary tables? Please correct me if I'm wrong in my assumption? PS5, Line 374: trace This seems like a serious enough error to me, basically blocking updates on a table to the impalads. Higher severity level may be? Line 970: } I see you made changes in the CatalogOpEx.drop*() to add the corresponding objects to the deltaLog_. Isn't it easier if we do it inside remove*() functions? Basically its up the Catalog to maintain its own state (detlaLog_) rather than clients updating it? That way deltaLog_ can remain private too? Else anyone can getDeleteLog() and mess with it. http://gerrit.cloudera.org:8080/#/c/7731/5/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java: Line 322: * Note that drop operations that come from statestore heartbeats always have a Update? -- To view, visit http://gerrit.cloudera.org:8080/7731 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93cb7a033dc8f0d3e0339394b36affe14523274c Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-HasComments: Yes
