> On Jan. 30, 2017, 11:24 p.m., Alexander Kolbasov wrote: > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java, > > line 86 > > <https://reviews.apache.org/r/55999/diff/1/?file=1616984#file1616984line86> > > > > This can be converted to foreach loop
partitions is an iterator. This is typical use of iterator. > On Jan. 30, 2017, 11:24 p.m., Alexander Kolbasov wrote: > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java, > > line 156 > > <https://reviews.apache.org/r/55999/diff/1/?file=1616984#file1616984line156> > > > > This can be converted to foreach loop partitions is an iterator. This is typical use of iterator. > On Jan. 30, 2017, 11:24 p.m., Alexander Kolbasov wrote: > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java, > > line 43 > > <https://reviews.apache.org/r/55999/diff/1/?file=1616983#file1616983line43> > > > > It isn't that we can't construct it, we can not deserialize it. > > > > Same goes for methods below. This is just replica of JSONMessageDeserializer: https://github.com/apache/hive/blob/master/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONMessageDeserializer.java > On Jan. 30, 2017, 11:24 p.m., Alexander Kolbasov wrote: > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java, > > line 42 > > <https://reviews.apache.org/r/55999/diff/1/?file=1616983#file1616983line42> > > > > readValue throws some specific exceptions - can we be more specific > > here as well? This is just replica of JSONMessageDeserializer: https://github.com/apache/hive/blob/master/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONMessageDeserializer.java > On Jan. 30, 2017, 11:24 p.m., Alexander Kolbasov wrote: > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java, > > line 32 > > <https://reviews.apache.org/r/55999/diff/1/?file=1616983#file1616983line32> > > > > IMO this can be removed The default constructor is required for Jackson. > On Jan. 30, 2017, 11:24 p.m., Alexander Kolbasov wrote: > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java, > > line 29 > > <https://reviews.apache.org/r/55999/diff/1/?file=1616982#file1616982line29> > > > > Can this be private and final? locations cannot be private final as the default - SentryJSONDropPartitionMessage() it required for jackson. - Nachiket ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55999/#review163576 ----------------------------------------------------------- On Jan. 26, 2017, 10:49 p.m., Nachiket Vaidya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55999/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2017, 10:49 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 > >
