Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-5538: Use explicit catalog versions for deleted objects ......................................................................
Patch Set 2: (16 comments) http://gerrit.cloudera.org:8080/#/c/7731/2/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: PS2, Line 233: 0, if they have then skip this step and use that data > nit: perhaps easier to read? "...version 0. If so, then skip ..." Done Line 305: BuildTopicUpdatesHelper(catalog_objects.updated_objects, false); > I like the previous version more where this is inlined into L292. A quick c Done Line 317: if (catalog_object.catalog_version <= last_sent_catalog_version_) continue; > Move this above L312? Done PS2, Line 336: if (topic_deletions) { : VLOG(1) << "Publishing deletion: " << entry_key << "@" : << catalog_object.catalog_version; : } else { : VLOG(1) << "Publishing update: " << entry_key << "@" : << catalog_object.catalog_version; : } > perhaps remove some redundancy this way: Done http://gerrit.cloudera.org:8080/#/c/7731/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 1340: LOG(INFO) << "Received large catalog update(>100mb): " > Received a large catalog item? Done PS2, Line 1366: if (item.deleted) { : VLOG(3) << "Deleted item: " << item.key << " version: " : << catalog_object.catalog_version; : } else { : VLOG(3) << "Added item: " << item.key << " version: " : << catalog_object.catalog_version; : } > same suggestion as in catalog_server to reduce redundancy. Done http://gerrit.cloudera.org:8080/#/c/7731/2/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: Line 86: 3: required bool deleted = false; > Isn't it better to not have a default value to make sure this is always set Done http://gerrit.cloudera.org:8080/#/c/7731/2/fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java File fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java: Line 56: * determine which catalog objects were deleted since the last catalog update topic. > catalog topic update Done Line 57: * Once the catalog update topic is constructed, the old deleted catalog objects are > catalog topic update Done Line 74: // Retrieve all the removed catalog objects with version > 'fromVersion'. > /** style comment Done http://gerrit.cloudera.org:8080/#/c/7731/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: PS2, Line 152: . > the word "delta" implies that we're dealing with inserts, deletes, and poss Done Line 347: // Identify the catalog objects that were updated (added/dropped) in the catalog with > /** style comment Done Line 351: Set<String> addedCatalogObjectKeys = Sets.newHashSet(); > updatedCatalogObjects? Done Line 363: TCatalogObject catalogTbl = new TCatalogObject(TCatalogObjectType.TABLE, > Why not just iterate over db.getTables()? Done Line 441: addedCatalogObjectKeys.add(CatalogDeltaLog.toCatalogObjectKey(privilege)); > Instead of having a duplicate add() in all places, how about creating the a Done Line 448: for (TCatalogObject removedObject: deltaLog_.retrieveObjects(fromVersion)) { > Nice! Much clearer. Done -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-HasComments: Yes
