impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
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:

(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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message