commit 6cb9f5564845264f8c9143be1e8986235b467f70 Author: Vihang Karajgaonkar Date: Mon Mar 26 10:17:23 2018 -0700 HIVE-18885 : DbNotificationListener has a deadlock between Java and DB locks (2.x line) diff --git a/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java b/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java index 41347c22df21a678241edeb766264e6d19c7885a..3376f18d4ceb93779198ed7fec4e9b690fc15f3d 100644 --- a/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java +++ b/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java @@ -71,20 +71,29 @@ * An implementation of {@link org.apache.hadoop.hive.metastore.MetaStoreEventListener} that * stores events in the database. * - * Design overview: This listener takes any event, builds a NotificationEventResponse, - * and puts it on a queue. There is a dedicated thread that reads entries from the queue and - * places them in the database. The reason for doing it in a separate thread is that we want to - * avoid slowing down other metadata operations with the work of putting the notification into - * the database. Also, occasionally the thread needs to clean the database of old records. We - * definitely don't want to do that as part of another metadata operation. + * Design overview: This listener takes a ListenerEvent, builds a NotificationEvent, + * and puts stores it in the database. When a NotificationEvent is created, an unique and monotonically + * increasing EVENT_ID is generated from the database and it assigned to each NotificationEvent. + * It is important to note that this Listener can be configured as a transaction listener + * in cases where client applications rely on the fact that the event is generated only when + * the metadata transaction is successfull. These strict contraints (monotonically increasing event id, + * with no holes and generated only when transaction successfull) can potentially cause a + * performance hit in a highly concurrent HMS environment + * + * Also, there is a cleaner thread which deletes old notification events at a regular interval. The + * time-to-live for the Notification Events can be configured by setting + * hive.metastore.event.db.listener.timetolive appropriately + * + * TODO : The code to generate EVENT_ID gets a database row lock on the NOTIFICATION_SEQUENCE + * table which blocks all the other metadata transactions until the event is committed along with + * parent transaction. This is can cause performance problem and the design needs to be improved + * */ public class DbNotificationListener extends MetaStoreEventListener { private static final Logger LOG = LoggerFactory.getLogger(DbNotificationListener.class.getName()); private static CleanerThread cleaner = null; - private static final Object NOTIFICATION_TBL_LOCK = new Object(); - // This is the same object as super.conf, but it's convenient to keep a copy of it as a // HiveConf rather than a Configuration. private HiveConf hiveConf; @@ -478,19 +487,17 @@ private int now() { * DB_NOTIFICATION_EVENT_ID_KEY_NAME for future reference by other listeners. */ private void process(NotificationEvent event, ListenerEvent listenerEvent) throws MetaException { + //no need for a synchronized block since synchronization accross multiple threads is done + //at database level in addNotificationEvent method. event.setMessageFormat(msgFactory.getMessageFormat()); - synchronized (NOTIFICATION_TBL_LOCK) { - LOG.debug("DbNotificationListener: Processing : {}:{}", event.getEventId(), - event.getMessage()); - HMSHandler.getMSForConf(hiveConf).addNotificationEvent(event); - } + LOG.debug("DbNotificationListener: Processing : {}:{}", event.getEventId(), event.getMessage()); + HMSHandler.getMSForConf(hiveConf).addNotificationEvent(event); - // Set the DB_NOTIFICATION_EVENT_ID for future reference by other listeners. - if (event.isSetEventId()) { - listenerEvent.putParameter( - MetaStoreEventListenerConstants.DB_NOTIFICATION_EVENT_ID_KEY_NAME, - Long.toString(event.getEventId())); - } + // Set the DB_NOTIFICATION_EVENT_ID for future reference by other listeners. + if (event.isSetEventId()) { + listenerEvent.putParameter(MetaStoreEventListenerConstants.DB_NOTIFICATION_EVENT_ID_KEY_NAME, + Long.toString(event.getEventId())); + } } private static class CleanerThread extends Thread { @@ -509,9 +516,7 @@ private void process(NotificationEvent event, ListenerEvent listenerEvent) throw @Override public void run() { while (true) { - synchronized(NOTIFICATION_TBL_LOCK) { - rs.cleanNotificationEvents(ttl); - } + rs.cleanNotificationEvents(ttl); LOG.debug("Cleaner thread done"); try { Thread.sleep(sleepTime);