impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5538: Use explicit catalog versions for deleted objects
Date Thu, 14 Sep 2017 22:50:32 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5538: Use explicit catalog versions for deleted objects

Patch Set 5:

File be/src/catalog/catalog-server.h:

PS5, Line 150: added
nit: May be 'added/updated/removed' to be consistent with other places?
File be/src/service/

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
File fe/src/main/java/org/apache/impala/catalog/

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).
File fe/src/main/java/org/apache/impala/catalog/

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.
File fe/src/main/java/org/apache/impala/catalog/

Line 322:    *  Note that drop operations that come from statestore heartbeats always have

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I93cb7a033dc8f0d3e0339394b36affe14523274c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-HasComments: Yes

View raw message