> On July 11, 2016, 8:33 p.m., Hao Hao wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java, > > line 25 > > <https://reviews.apache.org/r/49397/diff/8/?file=1440300#file1440300line25> > > > > Rename to newLocation?
Yea, I thought about but was not sure if I should change the variable name as we still need to support getLocation(), for the sake of other clients using default serializer. So if we change, getLocation will return newLocation. > On July 11, 2016, 8:33 p.m., Hao Hao wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java, > > line 100 > > <https://reviews.apache.org/r/49397/diff/8/?file=1440306#file1440306line100> > > > > Why this is false? Should it be true? Good question! I dont think we want to have a hard failure if there is an unknown field in the message, so we cannot just have a true here. But would be good for us to know that an extra field has been added in DbNotificationListener. So may be we should implement a DeserializationProblemHandler, and print an error from that handler. So that we can write a test to catch these when ever we change the Hive dependency? If you agree, I would prefer to create another jira to handle. > On July 11, 2016, 8:33 p.m., Hao Hao wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java, > > line 270 > > <https://reviews.apache.org/r/49397/diff/8/?file=1440314#file1440314line270> > > > > Do we need to add alter table with properties? And also alter table > > with column? Yeah, we could. I am mostly focusing on operations which change location for now. But would be good to add coverage for non location changing commands as well. Although, does that mean adding coverage for every possible Hive command? Either ways, I do agree alter table properties and alter table with columns are good cases, as we do want to make sure they do not cause a location validation failure. - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49397/#review141754 ----------------------------------------------------------- On July 9, 2016, 8:38 a.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49397/ > ----------------------------------------------------------- > > (Updated July 9, 2016, 8:38 a.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 > >
