Huaisi Xu has posted comments on this change. Change subject: IMPALA-3499: Split catalog update ......................................................................
Patch Set 9: (14 comments) for local testing i just started the cluster and saw everything is "fine". http://gerrit.cloudera.org:8080/#/c/3067/9/be/src/service/frontend.h File be/src/service/frontend.h: PS9, Line 38: The TUpdateCatalogCacheRequest contains : /// a list of objects that should be added/removed from the Catalog. Returns a response : /// that contains details such as the new max catalog version. > Update the comment Done PS9, Line 178: 2D_ > I don't think we need to change the name of this jmethod. Done http://gerrit.cloudera.org:8080/#/c/3067/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 123: > will remove Done PS9, Line 197: MAX_CATALOG_CACHE_UPDATE_SIZE > With this new patch, this no longer represents the maximum size of a catalo Done PS9, Line 1243: Update is split in pieces with size limit to : // be max_catalog_cache_update_size for multiple updates > nit: Maybe we should just say "An update is split into batches of size MAX. Done PS9, Line 1278: LOG(WARNING) > This should be VLOG(1) or 2. I have a feeling that VLOG(1) or 2 means no log.. I never saw anyone(including customers) enable this.. will just remove these to make it cleaner.. PS9, Line 1284: push_back(update_req); > Isn't this calling the copy constructor of TUpdateCatalogCacheRequest? If s Done Line 1291: update_reqs.push_back(update_req); > I believe we do, otherwise the size of the current update_req may be close Yes. just looks bad.. http://gerrit.cloudera.org:8080/#/c/3067/9/be/src/util/jni-util.h File be/src/util/jni-util.h: PS9, Line 275: _2D > I just made up something randomly.. I know I am poor at naming things.. :-) Done PS9, Line 286: jbytearray_2D > I don't think SetObjectArrayElement() would copy the bytes - the byte buffe I tested locally by manually corrupting the memory which request_bytes points to after line 285 and frontend sees corrupted update. perhaps these two are all java objects so they are copied by reference. :) http://gerrit.cloudera.org:8080/#/c/3067/9/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java: PS9, Line 160: cur_update > You're in Java world :) maybe catalogUpdate? Done PS9, Line 161: incremental_req > incrementalRequest Done PS9, Line 168: req.getUpdated_objects() == null > if (!req.isSetUpdated_objects()) I tried but I got bad operand type java.util.List<com.cloudera.impala.thrift.TCatalogObject> for unary operator '!'. I think that is a null object? no "!" for this object? PS9, Line 173: if (req.getRemoved_objects() == null) { > if (!req.isSetRemoved_objects()) see above -- 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: 9 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-HasComments: Yes
