You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Sergio Pena via Review Board <no...@reviews.apache.org> on 2018/03/08 16:11:49 UTC
Review Request 65985: HIVE-18783: ALTER TABLE post-commit listener
does not include the transactional listener responses
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/
-----------------------------------------------------------
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
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
> On March 12, 2018, 4:45 p.m., Na Li 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 when db name is changed at alter table, the transactionalListenerResponses is not past into MetaStoreListenerNotifier.notifyEvent()? And the command won't be able to block until sentry gets the notification of alter table.
Good catch. I added other variables for each transaction response.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/#review199027
-----------------------------------------------------------
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
>
>
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/#review199027
-----------------------------------------------------------
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
Lines 372 (patched)
<https://reviews.apache.org/r/65985/#comment279313>
why when db name is changed at alter table, the transactionalListenerResponses is not past into MetaStoreListenerNotifier.notifyEvent()? And the command won't be able to block until sentry gets the notification of alter table.
- Na Li
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
>
>
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
> On April 4, 2018, 12:34 a.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
> > Lines 408 (patched)
> > <https://reviews.apache.org/r/65985/diff/2/?file=1976883#file1976883line409>
> >
> > why do we need !success check here?
Ah? Thanks, I missed this. I removed the first condition and leave this one instead.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/#review200405
-----------------------------------------------------------
On March 15, 2018, 5:45 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65985/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 5:45 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 0dd3eb101709969a77998e1488e1c97214426cd3
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 66353e769b2d633ad9dfad2bcae25e8ad90f61d1
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java e6de00100003bc1be12b2772e2e97102ed476cf5
>
>
> Diff: https://reviews.apache.org/r/65985/diff/2/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
> On April 4, 2018, 12:34 a.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
> > Lines 408 (patched)
> > <https://reviews.apache.org/r/65985/diff/2/?file=1976883#file1976883line409>
> >
> > why do we need !success check here?
>
> Sergio Pena wrote:
> Ah? Thanks, I missed this. I removed the first condition and leave this one instead.
>
> Vihang Karajgaonkar wrote:
> I thought we need the first condition and not the second condition for !success. Are we not generating the events only when the transaction is successful? Am I missing something?
There is a comment I left to answer that question. I've seen in other operations that an event is generated even on failed events. I don't know why this is needed. I left the event generation just to be consistent with those operations, however, if the event failed, then at least just an ALTER_TABLE Event should be generated, and not the DROP/CREATE/ADD partitions events.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/#review200405
-----------------------------------------------------------
On April 4, 2018, 4:06 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65985/
> -----------------------------------------------------------
>
> (Updated April 4, 2018, 4:06 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 5459554fec911b253583df1f528e5abf134f055b
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java ed1b8c5cc2a4cb974dd37419c6b5f0601a035232
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 8539fea42fa2743381833ab3137579caeac64672
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java f59f40bc33367cff7a8d0d24d0e200b16c2c30e5
>
>
> Diff: https://reviews.apache.org/r/65985/diff/3/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
> On April 4, 2018, 12:34 a.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
> > Lines 408 (patched)
> > <https://reviews.apache.org/r/65985/diff/2/?file=1976883#file1976883line409>
> >
> > why do we need !success check here?
>
> Sergio Pena wrote:
> Ah? Thanks, I missed this. I removed the first condition and leave this one instead.
I thought we need the first condition and not the second condition for !success. Are we not generating the events only when the transaction is successful? Am I missing something?
- Vihang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/#review200405
-----------------------------------------------------------
On April 4, 2018, 4:06 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65985/
> -----------------------------------------------------------
>
> (Updated April 4, 2018, 4:06 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 5459554fec911b253583df1f528e5abf134f055b
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java ed1b8c5cc2a4cb974dd37419c6b5f0601a035232
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 8539fea42fa2743381833ab3137579caeac64672
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java f59f40bc33367cff7a8d0d24d0e200b16c2c30e5
>
>
> Diff: https://reviews.apache.org/r/65985/diff/3/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/#review200405
-----------------------------------------------------------
Fix it, then Ship it!
LGTM. Just one minor comment below.
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
Lines 408 (patched)
<https://reviews.apache.org/r/65985/#comment281169>
why do we need !success check here?
- Vihang Karajgaonkar
On March 15, 2018, 5:45 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65985/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 5:45 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 0dd3eb101709969a77998e1488e1c97214426cd3
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 66353e769b2d633ad9dfad2bcae25e8ad90f61d1
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java e6de00100003bc1be12b2772e2e97102ed476cf5
>
>
> Diff: https://reviews.apache.org/r/65985/diff/2/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/
-----------------------------------------------------------
(Updated April 4, 2018, 4:06 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 (updated)
-----
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 5459554fec911b253583df1f528e5abf134f055b
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java ed1b8c5cc2a4cb974dd37419c6b5f0601a035232
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 8539fea42fa2743381833ab3137579caeac64672
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java f59f40bc33367cff7a8d0d24d0e200b16c2c30e5
Diff: https://reviews.apache.org/r/65985/diff/3/
Changes: https://reviews.apache.org/r/65985/diff/2-3/
Testing
-------
All tests passed.
Thanks,
Sergio Pena
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/
-----------------------------------------------------------
(Updated March 15, 2018, 5:45 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 (updated)
-----
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 0dd3eb101709969a77998e1488e1c97214426cd3
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 66353e769b2d633ad9dfad2bcae25e8ad90f61d1
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java e6de00100003bc1be12b2772e2e97102ed476cf5
Diff: https://reviews.apache.org/r/65985/diff/2/
Changes: https://reviews.apache.org/r/65985/diff/1-2/
Testing
-------
All tests passed.
Thanks,
Sergio Pena
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
> 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
>
>
Re: Review Request 65985: HIVE-18783: ALTER TABLE post-commit
listener does not include the transactional listener responses
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65985/#review199063
-----------------------------------------------------------
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
Lines 362 (patched)
<https://reviews.apache.org/r/65985/#comment279355>
Would it make sense to move this out to its own try/catch block? This would avoid confusion with transaction try/catch block.
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
Lines 365 (patched)
<https://reviews.apache.org/r/65985/#comment279358>
This may cause weird situation where operation succeeds and is commitetd but throws an exception.
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
Lines 366 (patched)
<https://reviews.apache.org/r/65985/#comment279356>
It may be easier to read if you use
if (listeners.isEmpty()) {
return;
}
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
Lines 372 (patched)
<https://reviews.apache.org/r/65985/#comment279357>
Why are we creating dropTable/createTable notifications here? I understand that this mimics the code above, but it looks suspicious.
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java
Lines 99 (patched)
<https://reviews.apache.org/r/65985/#comment279354>
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?
- Alexander Kolbasov
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
>
>