> On July 8, 2016, 6:29 p.m., Hao Hao wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java, > > line 81 > > <https://reviews.apache.org/r/49397/diff/6/?file=1439091#file1439091line81> > > > > Why 999999 here?
Good question. I tried to retain some little details from DbNotificationListener. This is one of those: https://github.com/apache/hive/blob/4cbc4a652abe87742ccb4ea93e9ddfadce7d3e94/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java#L72 Seems like the "id" is never used. Will add a comment here based on my limited hive knowledge. > On July 8, 2016, 6:29 p.m., Hao Hao wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java, > > line 152 > > <https://reviews.apache.org/r/49397/diff/6/?file=1439091#file1439091line152> > > > > Why 0L for the eventID and 0 for the eventTime? To signify it is of long type. > On July 8, 2016, 6:29 p.m., Hao Hao wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java, > > line 280 > > <https://reviews.apache.org/r/49397/diff/6/?file=1439091#file1439091line280> > > > > Could we do processing here to extract the needed location change > > information based on the variant? Actually thinking a bit more about it. Seems like we will need dbname, tablename, location for both newTable and oldTable in all those cases. It is just that they might be duplicates in some cases. Adding new validations in the next patch. > On July 8, 2016, 6:29 p.m., Hao Hao wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java, > > line 423 > > <https://reviews.apache.org/r/49397/diff/6/?file=1439091#file1439091line423> > > > > Why not use the same unit for ttl and METASTORE_EVENT_DB_LISTENER_TTL? This is another thing which I borrowed from DbNotificationListener. We could change it but might be better to keep it consistent unless we know for sure, as we do intend to move back? > On July 8, 2016, 6:29 p.m., Hao Hao wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java, > > line 435 > > <https://reviews.apache.org/r/49397/diff/6/?file=1439091#file1439091line435> > > > > Should we make it configurable? Same as above. - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49397/#review141307 ----------------------------------------------------------- On July 7, 2016, 9:57 p.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49397/ > ----------------------------------------------------------- > > (Updated July 7, 2016, 9:57 p.m.) > > > Review request for sentry, Anne Yu, Colin McCabe, Hao Hao, and Nachiket > Vaidya. > > > Bugs: Sentry-1329 > https://issues.apache.org/jira/browse/Sentry-1329 > > > Repository: sentry > > > Description > ------- > > This patch adapts SentryMetaStorePostEventListener to write HMS notification > logs. Also, > 1. Implements the SentryJSONMessageFactory to add custom information in the > notification log entry message, which includes > 1.1. Implementing Message class for each message type > 1.2. Implementing a deserializer > 2. Implements JSONAlterPartitionMessage and JSONAlterTableMessage to work > around the issue in Hive 1.1.0. > 3. Testing: > 3.1. Sentry functionality: TestSentryListenerSentryDeserializer to verify > functionality using Sentry's SentryMetastorePostEventListener and Sentry > Notification log deserializer. > 3.2. TestDbNotificationListenerSentryDeserializer uses Hive's > DbNotificationListener and Sentry's JSON deserializeri. This would make sure > Sentry is able to read the Notification logs written by Hive's > DBNotificationListener > 3.3. TestSentryListenerInBuiltDeserializer uses Sentry's > SentryMetastorePostEventListener and Hive's inbuilt Notification log > deserializer: This would make sure Sentry is not breaking other users of > NotificationLog who might be using Hive's in built serializer > > > > Change-Id: I680beb6db4e534bb0a9e6ee042ea0d4f33f0943f > > update > > Change-Id: I6ce8846a801b3effc6a851ab3ce6f866b4a0c23e > > > Diffs > ----- > > sentry-binding/sentry-binding-hive/pom.xml > 07aaae3bac34582fe8222ff166a52c1e208b223a > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java > d12ac151ddbf35d612fee3f869d602d6cdf54aa8 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterPartitionMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterTableMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAddPartitionMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateDatabaseMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateTableMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropDatabaseMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropTableMessage.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java > 767bcbe02e0d511f52bc869e7b6a1ee1e6584a5a > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > 2c4948e9d100f2cf0cb5b7772489194c62b8a857 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java > b72e317621610eababf5d01886bc00dca9f5e7ec > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestDbNotificationListenerSentryDeserializer.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestHMSNotificationLogUsingDBNotificationListener.java > 0b328d402d005d6f858e87fbe64eedd5f9f1a092 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerInBuiltDeserializer.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/49397/diff/ > > > Testing > ------- > > Added new tests > > > Thanks, > > Sravya Tirukkovalur > >
