Repository: hive Updated Branches: refs/heads/branch-2 5842fd2f1 -> 29ad069c3
HIVE-18885 : DbNotificationListener has a deadlock between Java and DB locks (2.x line) (Vihang Karajgaonkar, reviewed by Alexander Kolbasov and Peter Vary) Project: http://git-wip-us.apache.org/repos/asf/hive/repo Commit: http://git-wip-us.apache.org/repos/asf/hive/commit/29ad069c Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/29ad069c Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/29ad069c Branch: refs/heads/branch-2 Commit: 29ad069c35821ed8d3a79c64cd6be5f5b0024ea6 Parents: 5842fd2 Author: Vihang Karajgaonkar <vih...@cloudera.com> Authored: Tue Mar 27 15:47:53 2018 -0700 Committer: Vihang Karajgaonkar <vih...@cloudera.com> Committed: Tue Mar 27 15:47:53 2018 -0700 ---------------------------------------------------------------------- .../listener/DbNotificationListener.java | 59 ++++++++++++-------- 1 file changed, 35 insertions(+), 24 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hive/blob/29ad069c/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java ---------------------------------------------------------------------- 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 41347c2..0cf51fb 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 @@ -68,23 +68,38 @@ import org.slf4j.LoggerFactory; import com.google.common.collect.Lists; /** - * An implementation of {@link org.apache.hadoop.hive.metastore.MetaStoreEventListener} that - * stores events in the database. + * <p>This listener takes a ListenerEvent, builds a NotificationEvent, + * and stores it in the database.This class can be configured as a listener (transaction or non-transactional) so that the listener + * events are logged into the database. + * </p> * - * 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. + * <p>Design overview: This class implements{@link org.apache.hadoop.hive.metastore.MetaStoreEventListener} + * 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 successful. Each NotificationEvent requires a EVENT_ID which is generated + * using a RW row lock in the database in the method RawStore.addNotificationEvent. This means any + * other concurrent transaction thread trying to create a event at the same time will block until the + * lock is released. This is required to satisfy the strict constraints (monotonically increasing event id, + * with no holes and generated only when transaction successful) on the client side + * </p> + * <p> + * 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 + * </p> + * <p>TODO : The code to generate EVENT_ID gets a database R/W row lock using SELECT FOR UPDATE for the single row + * on the NOTIFICATION_SEQUENCE table which blocks all the other metadata transactions until the + * event is committed along with parent transaction. This is likely to cause performance problem and the + * design needs to be improved + * </p> */ 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 +493,17 @@ public class DbNotificationListener extends MetaStoreEventListener { * 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 +522,7 @@ public class DbNotificationListener extends MetaStoreEventListener { @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);
