> On March 13, 2018, 3:35 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java > > Lines 362 (patched) > > <https://reviews.apache.org/r/65985/diff/1/?file=1972648#file1972648line362> > > > > Would it make sense to move this out to its own try/catch block? This > > would avoid confusion with transaction try/catch block.
Yes, I moved it. > On March 13, 2018, 3:35 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java > > Lines 365 (patched) > > <https://reviews.apache.org/r/65985/diff/1/?file=1972648#file1972648line365> > > > > This may cause weird situation where operation succeeds and is > > commitetd but throws an exception. I moved it to its own try/catch to avoid such weird situation. > On March 13, 2018, 3:35 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java > > Lines 366 (patched) > > <https://reviews.apache.org/r/65985/diff/1/?file=1972648#file1972648line366> > > > > It may be easier to read if you use > > > > if (listeners.isEmpty()) { > > return; > > } I'd like to keep the if (!listeners.isEmpty()) in case more code is added in the future that should be executed after the finally or this if statement and avoid the return if listeners is empty. > On March 13, 2018, 3:35 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java > > Lines 372 (patched) > > <https://reviews.apache.org/r/65985/diff/1/?file=1972648#file1972648line372> > > > > Why are we creating dropTable/createTable notifications here? I > > understand that this mimics the code above, but it looks suspicious. I left a comment there. The reason is if a rename happens between databases, then a drop/create table events between both databases should happen. Honestly, I don't think that is necessary and an ALTER event should be enough, but this code is already there and I don't know would affect other clients using notifications. Perhaps this is something to consider on the new HMS v2 design. > On March 13, 2018, 3:35 a.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java > > Lines 99 (patched) > > <https://reviews.apache.org/r/65985/diff/1/?file=1972650#file1972650line99> > > > > There is only a single consumer of this interface - it seems odd to > > have an interface which is used in some special case. Should all access to > > listeners be converted to the interface? I don't know. All listeners are used in the HiveMetaStore class which contains the HMSHandler subclass. Being listeners and transctionalListeners a variable of HMSHandler, then the HiveMetaStore can call them directly without need the interface. The only consumer is the HiveAlterHandler which should be re-design in my opinion as all DDL operations are inside the HiveMetaStore and only the alter is in its own class. Could this also be considered in the HMS v2 design? - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65985/#review199063 ----------------------------------------------------------- On March 8, 2018, 4:11 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65985/ > ----------------------------------------------------------- > > (Updated March 8, 2018, 4:11 p.m.) > > > Review request for hive, Alexander Kolbasov, Sahil Takiar, and Vihang > Karajgaonkar. > > > Bugs: HIVE-18783 > https://issues.apache.org/jira/browse/HIVE-18783 > > > Repository: hive-git > > > Description > ------- > > HIVE-16164 introduced a mechanism to pass HMS notification events ID to the > post-commit listeners for all DDL operations, but it didn't add it to the > ALTER TABLE event. This patch in review adds the same behavior for ALTER > TABLE events. > > > Diffs > ----- > > > itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java > e0e29652da94bbdaca515a17955d1409824c1742 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java > 89354a2d34249903a9ff13c4ed913a68de93057e > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 662de9a66767f27f31998f14c68f854e59993ab6 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java > e6de00100003bc1be12b2772e2e97102ed476cf5 > > > Diff: https://reviews.apache.org/r/65985/diff/1/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Sergio Pena > >
