You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Janaki Lahorani via Review Board <no...@reviews.apache.org> on 2017/11/07 21:19:52 UTC
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/
-----------------------------------------------------------
(Updated Nov. 7, 2017, 9:19 p.m.)
Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
Repository: hive-git
Description
-------
HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
Diffs
-----
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java ccadac1ada6aaae884ab39f5d99e91b8c542404e
Diff: https://reviews.apache.org/r/63586/diff/3/
Testing
-------
Thanks,
Janaki Lahorani
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/#review190527
-----------------------------------------------------------
Should AlterHandler be Configurable?
- Alexander Kolbasov
On Nov. 8, 2017, 7:17 p.m., Janaki Lahorani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63586/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 7:17 p.m.)
>
>
> Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 921cfc00343807179340fbdf40f21e2a46d936ab
>
>
> Diff: https://reviews.apache.org/r/63586/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Janaki Lahorani
>
>
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
Posted by Janaki Lahorani via Review Board <no...@reviews.apache.org>.
> On Nov. 9, 2017, 1:08 a.m., Alexander Kolbasov wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java
> > Lines 37 (patched)
> > <https://reviews.apache.org/r/63586/diff/4/?file=1885054#file1885054line37>
> >
> > It is unclear from the test that that's what is testing, so it would be good to explain how your test actually tests for this.
Comments added to explain the test.
> On Nov. 9, 2017, 1:08 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
> > Line 109 (original), 109 (patched)
> > <https://reviews.apache.org/r/63586/diff/4/?file=1885055#file1885055line109>
> >
> > Can you add a comment at the top of the class explaining that only the handler's config should be used and never the local stashed config?
> >
> > Do we need to support local config at all?
Comments added.
- Janaki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/#review190522
-----------------------------------------------------------
On Nov. 9, 2017, 5:04 p.m., Janaki Lahorani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63586/
> -----------------------------------------------------------
>
> (Updated Nov. 9, 2017, 5:04 p.m.)
>
>
> Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 921cfc00343807179340fbdf40f21e2a46d936ab
>
>
> Diff: https://reviews.apache.org/r/63586/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Janaki Lahorani
>
>
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
Posted by Janaki Lahorani via Review Board <no...@reviews.apache.org>.
> On Nov. 9, 2017, 1:08 a.m., Alexander Kolbasov wrote:
> > With this change, do we still need Config stored in the HiveAlterHandle itself? What is the value of returning such config in getConf() Should the new HiveAlterHandle extend COnfigurable? Do we need setConf method?
AlterHandler is a public interface. Changing that to not extend Configurable is a significant change, arguable as backward incompatible. This is something that will result in cleaner code, but not necessarily needed for the bug to be fixed. So, I chose not to change AlterHandler.
- Janaki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/#review190522
-----------------------------------------------------------
On Nov. 8, 2017, 7:17 p.m., Janaki Lahorani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63586/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 7:17 p.m.)
>
>
> Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 921cfc00343807179340fbdf40f21e2a46d936ab
>
>
> Diff: https://reviews.apache.org/r/63586/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Janaki Lahorani
>
>
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/#review190522
-----------------------------------------------------------
With this change, do we still need Config stored in the HiveAlterHandle itself? What is the value of returning such config in getConf() Should the new HiveAlterHandle extend COnfigurable? Do we need setConf method?
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java
Lines 37 (patched)
<https://reviews.apache.org/r/63586/#comment267971>
It is unclear from the test that that's what is testing, so it would be good to explain how your test actually tests for this.
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
Line 109 (original), 109 (patched)
<https://reviews.apache.org/r/63586/#comment267972>
Can you add a comment at the top of the class explaining that only the handler's config should be used and never the local stashed config?
Do we need to support local config at all?
- Alexander Kolbasov
On Nov. 8, 2017, 7:17 p.m., Janaki Lahorani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63586/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 7:17 p.m.)
>
>
> Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 921cfc00343807179340fbdf40f21e2a46d936ab
>
>
> Diff: https://reviews.apache.org/r/63586/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Janaki Lahorani
>
>
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
Posted by Janaki Lahorani via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/
-----------------------------------------------------------
(Updated Nov. 9, 2017, 5:04 p.m.)
Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
Changes
-------
Addressed comments from Alexander Kolbasov.
Repository: hive-git
Description
-------
HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
Diffs (updated)
-----
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 921cfc00343807179340fbdf40f21e2a46d936ab
Diff: https://reviews.apache.org/r/63586/diff/5/
Changes: https://reviews.apache.org/r/63586/diff/4-5/
Testing
-------
Thanks,
Janaki Lahorani
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
Posted by Andrew Sherman via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/#review190483
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Sherman
On Nov. 8, 2017, 7:17 p.m., Janaki Lahorani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63586/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 7:17 p.m.)
>
>
> Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 921cfc00343807179340fbdf40f21e2a46d936ab
>
>
> Diff: https://reviews.apache.org/r/63586/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Janaki Lahorani
>
>
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
Posted by Janaki Lahorani via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/
-----------------------------------------------------------
(Updated Nov. 8, 2017, 7:17 p.m.)
Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
Changes
-------
Addressed comments from Andrew.
Repository: hive-git
Description
-------
HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
Diffs (updated)
-----
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 921cfc00343807179340fbdf40f21e2a46d936ab
Diff: https://reviews.apache.org/r/63586/diff/4/
Changes: https://reviews.apache.org/r/63586/diff/3-4/
Testing
-------
Thanks,
Janaki Lahorani
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
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/63586/#review190457
-----------------------------------------------------------
Ship it!
LGTM. Thanks for the changes!
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java
Lines 94 (patched)
<https://reviews.apache.org/r/63586/#comment267837>
I looked up online I think a better way to do this would be Assert.fail("Exception not thrown"), but this works too.
- Vihang Karajgaonkar
On Nov. 7, 2017, 9:19 p.m., Janaki Lahorani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63586/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2017, 9:19 p.m.)
>
>
> Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java ccadac1ada6aaae884ab39f5d99e91b8c542404e
>
>
> Diff: https://reviews.apache.org/r/63586/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Janaki Lahorani
>
>
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
Posted by Janaki Lahorani via Review Board <no...@reviews.apache.org>.
> On Nov. 8, 2017, 5:03 p.m., Andrew Sherman wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java
> > Lines 107 (patched)
> > <https://reviews.apache.org/r/63586/diff/3/?file=1884157#file1884157line107>
> >
> > It is generally tidier to use the try-with-resources to close statements and connections. Older code doesn't always use this but we should probably make new code use it whenever possible.
Thanks Andrew. I changed it to do try-with-resources. I will be grateful if you can take a look.
- Janaki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/#review190454
-----------------------------------------------------------
On Nov. 8, 2017, 7:17 p.m., Janaki Lahorani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63586/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 7:17 p.m.)
>
>
> Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 921cfc00343807179340fbdf40f21e2a46d936ab
>
>
> Diff: https://reviews.apache.org/r/63586/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Janaki Lahorani
>
>
Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use
the conf from HMS Handler
Posted by Andrew Sherman via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63586/#review190454
-----------------------------------------------------------
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java
Lines 107 (patched)
<https://reviews.apache.org/r/63586/#comment267835>
It is generally tidier to use the try-with-resources to close statements and connections. Older code doesn't always use this but we should probably make new code use it whenever possible.
- Andrew Sherman
On Nov. 7, 2017, 9:19 p.m., Janaki Lahorani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63586/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2017, 9:19 p.m.)
>
>
> Review request for hive, Alexander Kolbasov, Andrew Sherman, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HMS handler thread local will have the configuration changes from the user local only to that connection. HiveAlterHandler should use the thread local to pick up user's configuration changes.
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java PRE-CREATION
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java ccadac1ada6aaae884ab39f5d99e91b8c542404e
>
>
> Diff: https://reviews.apache.org/r/63586/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Janaki Lahorani
>
>