You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Hao Hao <ha...@cloudera.com> on 2017/02/03 19:32:43 UTC
Re: Review Request 55999: SENTRY-1602 Code cleanup for Sentry JSON
message factory for hive notifications
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55999/#review164144
-----------------------------------------------------------
sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java (line 58)
<https://reviews.apache.org/r/55999/#comment235732>
While doing SENTRY-1587, I realize when we dropping Database, we also need the location of all the tables belong to this database. You can refer to my initial patch. Could you please add it here?
- Hao Hao
On Jan. 31, 2017, 11:27 p.m., Nachiket Vaidya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55999/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 11:27 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Hao Hao.
>
>
> Bugs: SENTRY-1602
> https://issues.apache.org/jira/browse/SENTRY-1602
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1602 Code cleanup for Sentry JSON message factory for hive notifications
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterPartitionMessage.java 890186ba7eecd4aace280d9a1b56a8f01b625b77
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterTableMessage.java 76211c35f4f5ed158b5c3052f7288cd0083f0d52
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java 99eb67a61363616af663a9be579b2e3a3344fd69
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java 6e59e2568c8fb3526cd7a8ce29b08f5d8f5b0d62
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java 2ab61f7ad5d30a1edb454c3cb239532c736b52e6
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java b645c4504ce6e768b31e6f7a1a39b60f73e1ac64
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java 00e7db8ea4ccf92dae58869fcb21e6e2fcb27103
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java 567b4c86339a9494647dccc980df2b427225907f
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java 6f1886f942ca80ab4c356b81777d3ac336017292
>
> Diff: https://reviews.apache.org/r/55999/diff/
>
>
> Testing
> -------
>
> Ran unit test cases and they passed.
>
>
> Thanks,
>
> Nachiket Vaidya
>
>
Re: Review Request 55999: SENTRY-1602 Code cleanup for Sentry JSON
message factory for hive notifications
Posted by Hao Hao <ha...@cloudera.com>.
> On Feb. 3, 2017, 7:32 p.m., Hao Hao wrote:
> > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java, line 60
> > <https://reviews.apache.org/r/55999/diff/3/?file=1620736#file1620736line60>
> >
> > While doing SENTRY-1587, I realize when we dropping Database, we also need the location of all the tables belong to this database. You can refer to my initial patch. Could you please add it here?
>
> Nachiket Vaidya wrote:
> When drop database cascade is called, drop table notifications are created for all tables of the database along with drop database. So we are ok.
>
> Hao Hao wrote:
> From what I discovered, it seems onDropTable of MetaStoreEventListener is not triggered in case of dropping databse. So we may still need it.
Double checked, it looks like onDropTable is actaully being invoked in this case. So closing this comment.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55999/#review164144
-----------------------------------------------------------
On Feb. 4, 2017, 5:42 a.m., Nachiket Vaidya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55999/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2017, 5:42 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Hao Hao.
>
>
> Bugs: SENTRY-1602
> https://issues.apache.org/jira/browse/SENTRY-1602
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1602 Code cleanup for Sentry JSON message factory for hive notifications
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterPartitionMessage.java 890186ba7eecd4aace280d9a1b56a8f01b625b77
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterTableMessage.java 76211c35f4f5ed158b5c3052f7288cd0083f0d52
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAddPartitionMessage.java c0c469c1b1bfcd09494bb611a4a07911f5ef3192
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java 99eb67a61363616af663a9be579b2e3a3344fd69
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java 6e59e2568c8fb3526cd7a8ce29b08f5d8f5b0d62
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateDatabaseMessage.java ba19cbe42a242ea74f3ea9bf424e799e95d17b3f
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateTableMessage.java 57d11d22f2f7c099c8dd354a207dbb297e39be3f
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropDatabaseMessage.java 05f83f7effee07204e7b0597ac6faf1e0e2e6cdf
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java 2ab61f7ad5d30a1edb454c3cb239532c736b52e6
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropTableMessage.java 7005776425a65ab5fdb4b959391c3558000406f4
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java b645c4504ce6e768b31e6f7a1a39b60f73e1ac64
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java 00e7db8ea4ccf92dae58869fcb21e6e2fcb27103
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java f3cefd6a232bfb91db28f04bebcc98ab3c1ca658
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java 6f1886f942ca80ab4c356b81777d3ac336017292
>
> Diff: https://reviews.apache.org/r/55999/diff/
>
>
> Testing
> -------
>
> Ran unit test cases and they passed.
>
>
> Thanks,
>
> Nachiket Vaidya
>
>
Re: Review Request 55999: SENTRY-1602 Code cleanup for Sentry JSON
message factory for hive notifications
Posted by Nachiket Vaidya <nv...@cloudera.com>.
> On Feb. 3, 2017, 7:32 p.m., Hao Hao wrote:
> > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java, line 60
> > <https://reviews.apache.org/r/55999/diff/3/?file=1620736#file1620736line60>
> >
> > While doing SENTRY-1587, I realize when we dropping Database, we also need the location of all the tables belong to this database. You can refer to my initial patch. Could you please add it here?
When drop database cascade is called, drop table notifications are created for all tables of the database along with drop database. So we are ok.
- Nachiket
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55999/#review164144
-----------------------------------------------------------
On Jan. 31, 2017, 11:27 p.m., Nachiket Vaidya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55999/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 11:27 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Hao Hao.
>
>
> Bugs: SENTRY-1602
> https://issues.apache.org/jira/browse/SENTRY-1602
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1602 Code cleanup for Sentry JSON message factory for hive notifications
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterPartitionMessage.java 890186ba7eecd4aace280d9a1b56a8f01b625b77
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterTableMessage.java 76211c35f4f5ed158b5c3052f7288cd0083f0d52
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java 99eb67a61363616af663a9be579b2e3a3344fd69
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java 6e59e2568c8fb3526cd7a8ce29b08f5d8f5b0d62
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java 2ab61f7ad5d30a1edb454c3cb239532c736b52e6
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java b645c4504ce6e768b31e6f7a1a39b60f73e1ac64
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java 00e7db8ea4ccf92dae58869fcb21e6e2fcb27103
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java 567b4c86339a9494647dccc980df2b427225907f
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java 6f1886f942ca80ab4c356b81777d3ac336017292
>
> Diff: https://reviews.apache.org/r/55999/diff/
>
>
> Testing
> -------
>
> Ran unit test cases and they passed.
>
>
> Thanks,
>
> Nachiket Vaidya
>
>
Re: Review Request 55999: SENTRY-1602 Code cleanup for Sentry JSON
message factory for hive notifications
Posted by Hao Hao <ha...@cloudera.com>.
> On Feb. 3, 2017, 7:32 p.m., Hao Hao wrote:
> > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java, line 60
> > <https://reviews.apache.org/r/55999/diff/3/?file=1620736#file1620736line60>
> >
> > While doing SENTRY-1587, I realize when we dropping Database, we also need the location of all the tables belong to this database. You can refer to my initial patch. Could you please add it here?
>
> Nachiket Vaidya wrote:
> When drop database cascade is called, drop table notifications are created for all tables of the database along with drop database. So we are ok.
From what I discovered, it seems onDropTable of MetaStoreEventListener is not triggered in case of dropping databse. So we may still need it.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55999/#review164144
-----------------------------------------------------------
On Jan. 31, 2017, 11:27 p.m., Nachiket Vaidya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55999/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 11:27 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Hao Hao.
>
>
> Bugs: SENTRY-1602
> https://issues.apache.org/jira/browse/SENTRY-1602
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1602 Code cleanup for Sentry JSON message factory for hive notifications
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterPartitionMessage.java 890186ba7eecd4aace280d9a1b56a8f01b625b77
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterTableMessage.java 76211c35f4f5ed158b5c3052f7288cd0083f0d52
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java 99eb67a61363616af663a9be579b2e3a3344fd69
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java 6e59e2568c8fb3526cd7a8ce29b08f5d8f5b0d62
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java 2ab61f7ad5d30a1edb454c3cb239532c736b52e6
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java b645c4504ce6e768b31e6f7a1a39b60f73e1ac64
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java 00e7db8ea4ccf92dae58869fcb21e6e2fcb27103
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java 567b4c86339a9494647dccc980df2b427225907f
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java 6f1886f942ca80ab4c356b81777d3ac336017292
>
> Diff: https://reviews.apache.org/r/55999/diff/
>
>
> Testing
> -------
>
> Ran unit test cases and they passed.
>
>
> Thanks,
>
> Nachiket Vaidya
>
>