You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org> on 2018/04/04 00:34:37 UTC
Re: 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/#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>.
> 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
>
>