[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/12118 )
Change subject: IMPALA-7970 : Add support for metastore event based automatic invalidate ...................................................................... Patch Set 9: (34 comments) Latest patch works through some of the comments. Still working through the rest of the comments http://gerrit.cloudera.org:8080/#/c/12118/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12118/8//COMMIT_MSG@7 PS8, Line 7: IMPALA-7970 : Add support for metastore event based automatic invalidate > nit: wrap lines to less than 72 chars. done http://gerrit.cloudera.org:8080/#/c/12118/8//COMMIT_MSG@21 PS8, Line 21: The change also adds a new test class to test the basic functionality > Is the self-event detection coming in a separate patch? Yes, self-event detection is still a work in progress and would be done as part of IMPALA-7972 http://gerrit.cloudera.org:8080/#/c/12118/8/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/12118/8/be/src/common/global-flags.cc@246 PS8, Line 246: invalidate cached tab > This only invalidates right? (refresh has a different meaning in Impala's c Right now, its only invalidate but I plan to make use of refresh in IMPALA-7973. I will change it to invalidate since that is the current state of the feature. http://gerrit.cloudera.org:8080/#/c/12118/8/be/src/common/global-flags.cc@247 PS8, Line 247: These metastore events could > nit: rephrase to ...cached table metadata based on... done http://gerrit.cloudera.org:8080/#/c/12118/8/be/src/common/global-flags.cc@251 PS8, Line 251: \ > ..has.. done http://gerrit.cloudera.org:8080/#/c/12118/8/be/src/common/global-flags.cc@253 PS8, Line 253: "creation or removal of objects from metastore, catalogd adds or removes such " > Since this is a user-facing message, we should probably clarify that this i The first line of the message mentions metastore events. Do you think that is not clear to a user? Added one line which describes how the metastore events could be generated from external systems like Apache Hive or another instance of Impala cluster talking with the same metastore. http://gerrit.cloudera.org:8080/#/c/12118/8/be/src/common/global-flags.cc@260 PS8, Line 260: > nit: format to fewer lines like others. I had formatted it like the others, but the clang-format-diff script which I use to format the patch doesn't seem to like it. Any idea how can fix it automatically? http://gerrit.cloudera.org:8080/#/c/12118/8/be/src/common/global-flags.cc@258 PS8, Line 258: r the old generation to be almost " : "full and trigger an invalidation on recently unused tables"); : > Looks like this is already mentioned above. Also, is it needed on the coord removed the redundant line. Also, yes, not need on coordinators as of now. http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@229 PS8, Line 229: > Add a oneliner comment. Also, we use an "_" prefix for class variables. foo done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@292 PS8, Line 292: > nit: sounds vague, rephrase? added some more meat to this. Please suggest improvements if you think something more is needed in the text. Thanks! http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@292 PS8, Line 292: : /** : * Initializes Metastore event > I think we can get rid of the implementation detail here, remove? Good point. Added check and return early logic. http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@304 PS8, Line 304: + "interval is %d", eventPollingInterval)); : return; : } > Isn't this a fatal error? (Include the stack trace in the error log) done. Did you mean changing the log level to fatal as well? http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/Db.java@112 PS8, Line 112: synchronized (thriftDb_) { thriftDb_.setMetastore_db(msDb); } > This additional ad-hoc locking without explanation of how it integrates wit I see your point. Initially, I wanted to model the changes to metastore db object similar to how functions are added to its parameters. I noticed that Db class has public method to add a function which can modify the state of the metastoredb object parameters without taking the version lock. However, as I look at the its callers, I see that this method is used from CatalogServiceCatalog class which takes a version lock. Perhaps the methods in Db class which modify the hms database parameters should not be made public in such a case. I will make changes to the patch so that updates to metastore db object are done before taking version lock and incrementing the version number http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@45 PS8, Line 45: > Probably good to preface this with a what an "event" is with a pointer to H done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@47 PS8, Line 47: us DDL operations like create/alter/drop : * operations on database, table, partition etc. Each event has a unique incremental id and the : * generated events are be fetched from Metastore to get incremental updates to the metadata stored : * in > Maybe just rephrase to "we keep track of last synced event in each iteratio done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@51 PS8, Line 51: be g > MAX_EVENTS.... done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@51 PS8, Line 51: erated by external Metastore clients like Apache Hive or Apache Spark as well as other : * Impala clusters configured to talk with the same metastore. : * > Remove? removed http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@56 PS8, Line 56: * each event type. We keep track of the last synced event id in each polling iteration so the next > nit: Make this bullet points instead? (supported / non-supported) Done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@64 PS8, Line 64: A new table/database is created in Catalog respectively. The newly created table/database are : * Incomplete and should get loaded lazily when needed : * <li>DROP_TABLE, DROP_DATABASE</li> : * The table/database is dropped from catalog if they are present : * <li>ALTER_TABLE</li> : * In case of alter table event, currently the code issues a invalidate table command. There is a : * special case of this event in case of renames, where the old table is removed and a new : * IncompleteTable with the new name is created. : * <li>ALTER_DATABASE</li> > I think this is implementation detail can be omitted in the class comment ( removed this part of javadoc as suggested http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@84 PS8, Line 84: String METASTORE_NOTIFICATIONS_ADD_THRIFT > _CONFIG_KEY Done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@93 PS8, Line 93: private final ScheduledExecutorService scheduler_ = > nit: "_" for class members (multiple places) Done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@109 PS8, Line 109: // maximum number of events to poll in each RPC > oneliner comment. Done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@113 PS8, Line 113: CatalogServiceCatalog catalog, long startSyncFromId, long pollingFrequencyInSec) { : this.catalog_ = Precondi > this.catalog_ = Preconditions.checkNotNull(catalog); Done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@128 PS8, Line 128: at > update. Done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@139 PS8, Line 139: tring.format("Starting metastore event polling with interval %d seconds.", : pollingFreque > nit: String.format() Done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@147 PS8, Line 147: , > include stack trace Done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@156 PS8, Line 156: */ > Looks like the only caller is processHMSNotificationEvents(), can we reuse This is not needed. Removed this method. http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@174 PS8, Line 174: LOG.info(String.format("Received %d events. Start event id : %d", : response.getEvents().size(), lastSyncedEventId_)); : } catch (TException e) { : throw new MetastoreNotificationException( : "Unable to fetch notifications from metastore. Last synced event id is " : > Like I commented below, it appears to me that (lastSyncedEventId_ != -1) i This check is redundant and not required. Removed it. http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@182 PS8, Line 182: try { : processEvent(event); : } catch (MetastoreNotificationException ex) { : LOG.warn(String > Can we skip this and directly do a single RPC to fetch the latest events? The getCurrentNotificationEventId is much cheaper (and hence faster too) than fetching all the events on the HMS server side. I haven't profiled it but I think in the common case of not all the polling iterations detect new events, this implementation is faster than fetching events and making sure that events.size() > 0. I will add a comment since this is not obvious to the reader http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@187 PS8, Line 187: (), ex.getMessage > Looks like we are only doing only a single batch per iteration. Should we r Done http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@191 PS8, Line 191: // throwing exception until catalogD is restarted > Looks like this event is lost forever. Any idea how this can be surfaced ba This would happen in a rare case of a bug in the event serialization and deserialization or a misconfiguration on the HMS side. The processEvent throws a exception and it is logged as a error. I am not sure if there are better ways to surface this to user. I would be happy to implement if you have better suggestions. http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@196 PS8, Line 196: > If a single event processing throws an exception, we seem to be breaking ou They are not lost forever since the lastSycnedEventId is only updated one by one and we will request for these events again in the next batch. But yes, I think we can improve this to process to attempt to process the rest of the events in the batch. Fixed it. http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@212 PS8, Line 212: bName, > This seems to be optional in the thrift spec and the next line there is a ( I am not sure I understood this comment correctly. The db!=null check makes sure that catalogD has this db object since we rely on that to get the table object from catalog. The tblName could be null in the events like CREATE_DATABASE http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@215 PS8, Line 215: case "ALTER_TABLE": > Strings look risky, is there no EventType enum or some such? The eventTypes from the metastore API are strings. We can create our own enums which map to these strings if you think that is useful. -- To view, visit http://gerrit.cloudera.org:8080/12118 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic70b27780560b7ac9b33418d132b36cd0ca4abf7 Gerrit-Change-Number: 12118 Gerrit-PatchSet: 9 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 27 Dec 2018 00:58:52 +0000 Gerrit-HasComments: Yes
