Re: Review Request 70524: Break up DDLTask - extract Workload Management related operations

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70524/#review214816
---




ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterWMMappingOperation.java
Lines 28 (patched)


This is a nit, but if you spell out "workload management" rather than "wm" 
in the javadoc then the classes will be easier for the naive reader to 
understand.


- Andrew Sherman


On April 23, 2019, 1:23 p.m., Miklos Gergely wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70524/
> ---
> 
> (Updated April 23, 2019, 1:23 p.m.)
> 
> 
> Review request for hive and Zoltan Haindrich.
> 
> 
> Bugs: HIVE-21635
> https://issues.apache.org/jira/browse/HIVE-21635
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> DDLTask is a huge class, more than 5000 lines long. The related DDLWork is 
> also a huge class, which has a field for each DDL operation it supports. The 
> goal is to refactor these in order to have everything cut into more 
> handleable classes under the package  org.apache.hadoop.hive.ql.exec.ddl:
> 
> have a separate class for each operation
> have a package for each operation group (database ddl, table ddl, etc), so 
> the amount of classes under a package is more manageable
> make all the requests (DDLDesc subclasses) immutable
> DDLTask should be agnostic to the actual operations
> right now let's ignore the issue of having some operations handled by DDLTask 
> which are not actual DDL operations (lock, unlock, desc...)
> In the interim time when there are two DDLTask and DDLWork classes in the 
> code base the new ones in the new package are called DDLTask2 and DDLWork2 
> thus avoiding the usage of fully qualified class names where both the old and 
> the new classes are in use.
> 
> Step #6: extract all the workload management related operations from the old 
> DDLTask, and move them under the new package.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLWork2.java a2f49b7503 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/CreateRoleOperation.java 
> 6782b02d20 
>   ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/DropRoleOperation.java 
> e8b55ecf4c 
>   ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/GrantOperation.java 
> 633ac434e0 
>   ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/GrantRoleOperation.java 
> 19abe2794d 
>   ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/RevokeOperation.java 
> bf4e01a191 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/RevokeRoleOperation.java 
> 0b3b27695d 
>   ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/RoleUtils.java 
> cfbc4cf620 
>   ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/SetRoleOperation.java 
> d119fe4a28 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/ShowCurrentRoleOperation.java
>  9738ddbcc0 
>   ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/ShowGrantOperation.java 
> 50b41800a1 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/ShowPrincipalsOperation.java
>  392142ba14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/ShowRoleGrantOperation.java
>  178ea8e3bc 
>   ql/src/java/org/apache/hadoop/hive/ql/ddl/privilege/ShowRolesOperation.java 
> 22ca7f350d 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterPoolAddTriggerDesc.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterPoolAddTriggerOperation.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterPoolDropTriggerDesc.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterPoolDropTriggerOperation.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterResourcePlanOperation.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterWMMappingOperation.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterWMPoolDesc.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterWMPoolOperation.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/AlterWMTriggerOperation.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/CreateResourcePlanOperation.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/CreateWMMappingDesc.java
>  PRE-CREATION 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/workloadmanagement/CreateWMMappingOperation.java
>  PRE-CREATION 
>   
> 

Re: Review Request 69054: HIVE-20740 : Remove global lock in ObjectStore.setConf method

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69054/#review210893
---



LGTM


standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 1020 (patched)


nit: delete



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 1033 (patched)


nit: delete


- Andrew Sherman


On Nov. 27, 2018, 7:18 a.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69054/
> ---
> 
> (Updated Nov. 27, 2018, 7:18 a.m.)
> 
> 
> Review request for hive, Andrew Sherman, Alan Gates, and Peter Vary.
> 
> 
> Bugs: HIVE-20740
> https://issues.apache.org/jira/browse/HIVE-20740
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20740 : Remove global lock in ObjectStore.setConf method
> 
> 
> Diffs
> -
> 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenarios.java
>  5a88550f0625a7ec1890df7f54e7fa579f58fff4 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
> 5cb0a887e672f49739f5b648e608fba66de06326 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> 455ffc3887e62fa503cc3fa28255702ea9da3cc0 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  570281b54fa236d5bb568b4ded9b166ef367f613 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  af9efd98ea210335c6ac1d3da8624e02aadc2706 
> 
> 
> Diff: https://reviews.apache.org/r/69054/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 69167: HIVE-20796: jdbc URL can contain sensitive information that should not be logged

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69167/#review210043
---




standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
Lines 1176 (patched)


Nit: all the trendy kids use StringBuilder now


- Andrew Sherman


On Oct. 25, 2018, 1:36 p.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69167/
> ---
> 
> (Updated Oct. 25, 2018, 1:36 p.m.)
> 
> 
> Review request for hive and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20796: jdbc URL can contain sensitive information that should not be 
> logged
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  9c158040497cd3d2762620ce35e2b46bb6d5fffe 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
>  f3b38665676391fec9b85eb9a405c14632340dc6 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java
>  f4bdd734dc4e731dda01e6031a4115cde5571baf 
> 
> 
> Diff: https://reviews.apache.org/r/69167/diff/1/
> 
> 
> Testing
> ---
> 
> New unit test created.
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 69054: HIVE-20740 : Remove global lock in ObjectStore.setConf method

[Andrew Sherman via Review Board]

> On Oct. 16, 2018, 8:59 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 291 (patched)
> > 
> >
> > What is someone has set  ConfVars.MANAGER_FACTORY_CLASS to some 
> > non-default value? Is this still correct?
> 
> Vihang Karajgaonkar wrote:
> Yes, looks like it will fail in that case although I am not sure the 
> use-cases where you will use a different PersistenceManagerFactory. This code 
> has been there since before the patch and has not been changed in this patch. 
> Perhaps we can look at this as a separate JIRA.

OK with me


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69054/#review209662
---


On Oct. 19, 2018, 8:54 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69054/
> ---
> 
> (Updated Oct. 19, 2018, 8:54 p.m.)
> 
> 
> Review request for hive, Andrew Sherman, Alan Gates, and Peter Vary.
> 
> 
> Bugs: HIVE-20740
> https://issues.apache.org/jira/browse/HIVE-20740
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20740 : Remove global lock in ObjectStore.setConf method
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  66977d79c946f1ac57aacfbe8704d37bfbac3ea3 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  b74c3048fa2e18adc7f0d7cc813a180d4466fa36 
> 
> 
> Diff: https://reviews.apache.org/r/69054/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 69054: HIVE-20740 : Remove global lock in ObjectStore.setConf method

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69054/#review209662
---



This all looks good, I just have annoying questions


standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 290 (patched)


Can you use  ConfVars.MANAGER_FACTORY_CLASS.getVarname() instead of the 
string?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 291 (patched)


What is someone has set  ConfVars.MANAGER_FACTORY_CLASS to some non-default 
value? Is this still correct?



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 918 (patched)


Add comment expaining what the test does



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 923 (patched)


numThreads and numIterations seem small to me, can we make them higher 
without the test taking a long time?



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 927 (patched)


ArrayList<>(numThreads) ?



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 948 (patched)


nit: add a timeout to get then you will kow the test can never hang


- Andrew Sherman


On Oct. 16, 2018, 8:36 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69054/
> ---
> 
> (Updated Oct. 16, 2018, 8:36 p.m.)
> 
> 
> Review request for hive, Andrew Sherman, Alan Gates, and Peter Vary.
> 
> 
> Bugs: HIVE-20740
> https://issues.apache.org/jira/browse/HIVE-20740
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20740 : Remove global lock in ObjectStore.setConf method
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  66977d79c946f1ac57aacfbe8704d37bfbac3ea3 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  b74c3048fa2e18adc7f0d7cc813a180d4466fa36 
> 
> 
> Diff: https://reviews.apache.org/r/69054/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 68969: HIVE-20307 : Add support for filterspec to the getPartitions with projection API

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68969/#review209385
---


Ship it!





standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
Lines 718 (patched)


This is so much nicer


- Andrew Sherman


On Oct. 9, 2018, 9:23 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68969/
> ---
> 
> (Updated Oct. 9, 2018, 9:23 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
> 
> 
> Bugs: HIVE-20307
> https://issues.apache.org/jira/browse/HIVE-20307
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20307 : Add support for filterspec to the getPartitions with projection 
> API
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  d59d5d807a26378a430e683533e53d0831cf9514 
>   
> ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java
>  a2b57fb646899c54b63be14a8cde9b8644a973aa 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  16f4a50d69f9120d565f61d028b060d7776689fc 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  08614749b7aba54f9eb9b54ac46f79dbac6bc5cd 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  af757932a191675bc8fb9236209a2efba9f3d335 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  66977d79c946f1ac57aacfbe8704d37bfbac3ea3 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  a6d9583364be20758444ebe25c8cf636f0ea740f 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  70490f09e765d4e42391c67eb5cf018e93ad04aa 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  4dd4edccd66f8ea8ea189a2d27f970c8113e3a0f 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  06f4cbce58c16f98257e7f529ffe31c983f2919f 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
>  dcff606937157e63694657b42392875d50b17be6 
> 
> 
> Diff: https://reviews.apache.org/r/68969/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 68969: HIVE-20307 : Add support for filterspec to the getPartitions with projection API

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68969/#review209380
---



All looks fine within the bounds of what I understand


standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
Lines 608 (patched)


Add a default to case statement to catch future evil



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 4100 (patched)


This is a total nit but you can do this sort of thing in one line with 
UNTESTED AND UNTRIED stuff like
filters.stream().map(s -> "(" + s + ")").collect(Collectors.joining(" AND 
"));



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
Line 572 (original), 572 (patched)


spelling -> represent



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
Lines 645 (patched)


These tests that set a property in client assume stuff about the order of 
tests? Would it be safer for each test to undo the change it made to client at 
the end of the test?



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
Lines 738 (patched)


Maybe this boilerplate, which is repeated, should be put in a method? In 
fact the whole methid could use a load of helper methods to make it more 
readable.


- Andrew Sherman


On Oct. 9, 2018, 7:47 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68969/
> ---
> 
> (Updated Oct. 9, 2018, 7:47 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
> 
> 
> Bugs: HIVE-20307
> https://issues.apache.org/jira/browse/HIVE-20307
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20307 : Add support for filterspec to the getPartitions with projection 
> API
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  d59d5d807a26378a430e683533e53d0831cf9514 
>   
> ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java
>  a2b57fb646899c54b63be14a8cde9b8644a973aa 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  16f4a50d69f9120d565f61d028b060d7776689fc 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  08614749b7aba54f9eb9b54ac46f79dbac6bc5cd 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  af757932a191675bc8fb9236209a2efba9f3d335 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  66977d79c946f1ac57aacfbe8704d37bfbac3ea3 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  a6d9583364be20758444ebe25c8cf636f0ea740f 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  70490f09e765d4e42391c67eb5cf018e93ad04aa 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  4dd4edccd66f8ea8ea189a2d27f970c8113e3a0f 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  06f4cbce58c16f98257e7f529ffe31c983f2919f 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
>  dcff606937157e63694657b42392875d50b17be6 
> 
> 
> Diff: https://reviews.apache.org/r/68969/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 68664: HIVE-20306: Implement projection spec for fetching only requested fields from partitions

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68664/#review209171
---


Fix it, then Ship it!




All looks good, just a few nits which I noticed


standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
Line 1667 (original), 1667 (patched)


I think he previous text 'E.g.' is better



standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
Line 1673 (original), 1673 (patched)


Not sure why this changed, should be 'compliant'



standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
Line 1676 (original), 1676 (patched)


Not sure why this changed, should be 'compliant'



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
Lines 3765 (patched)


Arguably clearer to say "providing different ways of filtering and 
controlling output"


- Andrew Sherman


On Oct. 3, 2018, 12:07 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68664/
> ---
> 
> (Updated Oct. 3, 2018, 12:07 a.m.)
> 
> 
> Review request for hive, Aihua Xu, Andrew Sherman, Peter Vary, Todd Lipcon, 
> and Vihang Karajgaonkar.
> 
> 
> Bugs: HIVE-20306
> https://issues.apache.org/jira/browse/HIVE-20306
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20306: Implement projection spec for fetching only requested fields from 
> partitions
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  0ad2a2469e0330e050fdb8983078b80617afbbf1 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsFilterSpec.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsProjectSpec.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsRequest.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsResponse.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/PartitionFilterMode.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java
>  ae0956870a7d01c24f5fdaa07094c3dc6604ab9a 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/ThriftHiveMetastore.php
>  4574c6a4925ae3df9dd1ee7b8786976ae6fc8397 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/Types.php
>  22deffe1d31a64f95c49d7f017dfeb2994233e71 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote
>  a595732f04af4304974186178377192227bb80fb 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py
>  38074ce79b8a06b3795d00431025240778abb569 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ttypes.py
>  38fac465d73c264f85fc512548ebe1919ee35c17 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-rb/hive_metastore_types.rb
>  0192c6da314694c1253b49949bbe749902f49b4b 
>   
> standalone-metastore/metastore-common/src/gen/thrift/gen-rb/thrift_hive_metastore.rb
>  e6a72762bb7b0d36fdf6d20d02cb1da3337a98a0 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
>  d226db50a520707abee11fe7b81e2d95c39a6679 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
>  54e7eda0da796877f1331de137d534126375c6ba 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  a92f34bb1f149ab063c6f97b80b112f3a9e7f85e 
>   standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift 
> 85a5c601e03ecd2fb6ac5d30d789193e10bf38c2 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  ba82a9327cf18e8d55ebddcd774786d3d72f753a 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
>   
> 

Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener should not use /tmp directory

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/#review209165
---


Ship it!




Ship It!

- Andrew Sherman


On Oct. 2, 2018, 9:55 p.m., Bharathkrishna Guruvayoor Murali wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68889/
> ---
> 
> (Updated Oct. 2, 2018, 9:55 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
> 
> 
> Bugs: HIVE-20610
> https://issues.apache.org/jira/browse/HIVE-20610
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Adding java.io.tmpdir as tmp directory instead of /tmp
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
>  82429e36a575918d92a9b22bedcd63788ec51c5f 
> 
> 
> Diff: https://reviews.apache.org/r/68889/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bharathkrishna Guruvayoor Murali
> 
>

Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener should not use /tmp directory

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/#review209131
---




itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
Lines 123 (patched)


General comment: don't try to do too much in static initializers in server 
code. Just like in HIVE-20545 you have to consider what will happen if there is 
a failure during initialization, and the result is always ugly. In this case it 
looks safe but IT MADE ME THINK which is bad.



itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
Line 802 (original), 804 (patched)


This looks ugly to me. I think the string concatenation operator + should 
be separated on both sides by spaces. I think that is what is most commonly 
used on Hive - I'll leave it to you to check. But the usage is here is 
different from that in the static initializer code and that inconsistency is 
ugly too. IMHO You should teach Intellij to do your formatting and then let it 
decide this stuff


- Andrew Sherman


On Oct. 1, 2018, 7:09 a.m., Bharathkrishna Guruvayoor Murali wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68889/
> ---
> 
> (Updated Oct. 1, 2018, 7:09 a.m.)
> 
> 
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
> 
> 
> Bugs: HIVE-20610
> https://issues.apache.org/jira/browse/HIVE-20610
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Adding java.io.tmpdir as tmp directory instead of /tmp
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
>  82429e36a5 
> 
> 
> Diff: https://reviews.apache.org/r/68889/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bharathkrishna Guruvayoor Murali
> 
>

Re: Review Request 68827: HIVE-20545 : Exclude large-sized parameters from serialization of Table and Partition thrift objects in HMS notifications

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68827/#review209102
---




standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 517 (patched)


typo: th


- Andrew Sherman


On Sept. 28, 2018, 4:59 p.m., Bharathkrishna Guruvayoor Murali wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68827/
> ---
> 
> (Updated Sept. 28, 2018, 4:59 p.m.)
> 
> 
> Review request for hive and Alexander Kolbasov.
> 
> 
> Bugs: HIVE-20545
> https://issues.apache.org/jira/browse/HIVE-20545
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Clients can add large-sized parameters in Table/Partition objects. So we need 
> to enable adding regex patterns through HiveConf to match parameters to be 
> filtered from table and partition objects before serialization in HMS 
> notifications.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  30ea7f81292b0db54f4eb82468191fda38f9a0d4 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  c681a87a1c6b10a4f9494e49a42282cf90027ad7 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/messaging/json/JSONMessageFactory.java
>  2668b053205f48226da442ce65fcc2d7f6e76763 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java
>  30de1c4cfa1cf019186b10583a06da0bf5491634 
> 
> 
> Diff: https://reviews.apache.org/r/68827/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bharathkrishna Guruvayoor Murali
> 
>

Re: Review Request 68827: HIVE-20545 : Exclude large-sized parameters from serialization of Table and Partition thrift objects in HMS notifications

[Andrew Sherman via Review Board]

> On Sept. 26, 2018, 5:51 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
> > Lines 917 (patched)
> > 
> >
> > make private
> 
> Bharathkrishna Guruvayoor Murali wrote:
> I thought this could be used separately as a utility method, if the user 
> has just one predicate. Should I just keep the other method which accepts a 
> list of predicates and make this one private?

IDK, not super important


> On Sept. 26, 2018, 5:51 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/common/TestMetaStoreUtils.java
> > Lines 66 (patched)
> > 
> >
> > add test with the default param map from MetastoreConf
> 
> Bharathkrishna Guruvayoor Murali wrote:
> An empty exclude pattern will remove all elements from the map! But when 
> it is done through configuration, the default value returns an empty array, 
> like new String[0].
> Hence, the predicates become an empty List. What can I do if needed to 
> add a test for this.

What I mean is a test that works from a default  MetaConf object. This make the 
test more close to reality.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68827/#review209031
---


On Sept. 28, 2018, 10:17 a.m., Bharathkrishna Guruvayoor Murali wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68827/
> ---
> 
> (Updated Sept. 28, 2018, 10:17 a.m.)
> 
> 
> Review request for hive and Alexander Kolbasov.
> 
> 
> Bugs: HIVE-20545
> https://issues.apache.org/jira/browse/HIVE-20545
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Clients can add large-sized parameters in Table/Partition objects. So we need 
> to enable adding regex patterns through HiveConf to match parameters to be 
> filtered from table and partition objects before serialization in HMS 
> notifications.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  30ea7f81292b0db54f4eb82468191fda38f9a0d4 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  c681a87a1c6b10a4f9494e49a42282cf90027ad7 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/messaging/json/JSONMessageFactory.java
>  2668b053205f48226da442ce65fcc2d7f6e76763 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java
>  30de1c4cfa1cf019186b10583a06da0bf5491634 
> 
> 
> Diff: https://reviews.apache.org/r/68827/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bharathkrishna Guruvayoor Murali
> 
>

Re: Review Request 68827: Exclude large-sized parameters from serialization of Table and Partition thrift objects in HMS notifications

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68827/#review209031
---



Code looks clean, but I have some scary questions.


standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 517 (patched)


Make this clearer.
A list of comma separated regeses that are used to reduced the size of Hms 
Notifiaction messages.
If a partition name (?) or table name (?) matches a regex then the  
partition or table is excluded from the notification.
Or something



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 1412 (patched)


Add javadoc



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
Lines 913 (patched)


Good doumentation!
Are you 100% sure that this Map is never sused by anyone else? What about 
future code?



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
Lines 917 (patched)


make private



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/messaging/json/JSONMessageFactory.java
Lines 106 (patched)


So what if I make a miskae in a regex in config? This code will fail, what 
will happen then? How will I know what failed if my server won't start?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/messaging/json/JSONMessageFactory.java
Line 297 (original), 310 (patched)


This is used by the notifications that (we think) we understand, but it is 
also used by JSONAcidWriteMessage. So what happens if someone uses your new 
mechanism to reduce the size of messages, but affects JSONAcidWriteMessage? In 
other words there could be multile uses for notifications in a complex system, 
and this mechanism affects them all.



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/common/TestMetaStoreUtils.java
Lines 35 (patched)


Add javadoc



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/common/TestMetaStoreUtils.java
Lines 36 (patched)


We already have TestMetaStoreServerUtils, does this need a new file?



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/common/TestMetaStoreUtils.java
Lines 66 (patched)


add test with the default param map from MetastoreConf


- Andrew Sherman


On Sept. 24, 2018, 8:37 p.m., Bharathkrishna Guruvayoor Murali wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68827/
> ---
> 
> (Updated Sept. 24, 2018, 8:37 p.m.)
> 
> 
> Review request for hive and Alexander Kolbasov.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Clients can add large-sized parameters in Table/Partition objects. So we need 
> to enable adding regex patterns through HiveConf to match parameters to be 
> filtered from table and partition objects before serialization in HMS 
> notifications.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  30ea7f8129 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  c681a87a1c 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/messaging/json/JSONMessageFactory.java
>  2668b05320 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/common/TestMetaStoreUtils.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68827/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bharathkrishna Guruvayoor Murali
> 
>

Re: Review Request 68710: HIVE-20544: TOpenSessionReq logs password and username

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68710/#review208993
---




service-rpc/pom.xml
Lines 156 (patched)


This change works (I assume) but it is fragile. What if the generated code 
changes? Maybe consider 
1. adding a simple java unit test that proves that the password is not in 
toString() output 
2. adding a comment to the generated code so that readers can see that 
somethign funny is happening


- Andrew Sherman


On Sept. 24, 2018, 2:01 p.m., Karen Coppage wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68710/
> ---
> 
> (Updated Sept. 24, 2018, 2:01 p.m.)
> 
> 
> Review request for hive and Laszlo Pinter.
> 
> 
> Bugs: HIVE-20544
> https://issues.apache.org/jira/browse/HIVE-20544
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> TOpenSessionReq, if client protocol is unset, both username and password are 
> logged. Logging a password is a security risk. This patch would hide it with 
> asterisks.
> 
> 
> Diffs
> -
> 
>   service-rpc/pom.xml d6a07a55bc 
>   
> service-rpc/src/gen/thrift/gen-javabean/org/apache/hive/service/rpc/thrift/TOpenSessionReq.java
>  3195e704f3 
> 
> 
> Diff: https://reviews.apache.org/r/68710/diff/5/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> HIVE-20544.3.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/24/9f8ef0d8-22df-40cf-a311-56335d88516a__HIVE-20544.3.patch
> HIVE-20544.3.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/09/24/afdfc085-cc06-4a47-81f8-499029719bd0__HIVE-20544.3.patch
> 
> 
> Thanks,
> 
> Karen Coppage
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Lines 316 (patched)
> > 
> >
> > 1) Does setObject() work OK on all the jdbc drivers that are supported? 
> > In the oast I have seen cases where it was necessary to dispatch to the 
> > correct method like setString, setInt 
> > 2) can the params over be null? Do we need to call setNull instead of 
> > setObject()? Again we need to consider all the drivers.
> 
> Laszlo Pinter wrote:
> 1) The jdbc driver will do the type checking. A slight disadvantage is 
> the minor overhead, but this is negligible as compared to the better 
> maitainable code you end up with.
> 2) You're correct, I have to make sure that the params[i] is not null or 
> use setNull instead.
> 
> Laszlo Pinter wrote:
> So I did a bit more of a debugging, and my previous comment about the 
> params[i] can be null is not correct. The params can contain partitionIds, 
> storageDescriptorIds, columnDescriptorIds, serdeIds, depeding from where the 
> executeNoResult() is called.  These fields are mandatory and cannot be null. 
> If any of these items are null, means that the metastore db is not consistent 
> and it was corrupted.
> 
> Andrew Sherman wrote:
> I worte this once, but rb ate it, sorry if it duplicates.
> On 1) Did you test with all drivers?
> On 2) I suggest you add some checking to nail down that aprams are 
> non-null. How is the java testing of this class? Do we need negative test 
> cases?
> 
> Laszlo Pinter wrote:
> 1) Do you know what are the supported jdbc drivers or where could I check 
> them?
> 2) It doesn't makes sense to have null values in queries like 
> ```sql
> SELECT column_name1 FROM table_name WHERE column_name2 IN (value1, value2 
> ...);
> ```
> so I filtered them out.

2) OK
1) 
https://cwiki.apache.org/confluence/display/Hive/AdminManual+MetastoreAdmin#AdminManualMetastoreAdmin-SupportedBackendDatabasesforMetastore
http://www.cloudera.com/documentation/enterprise/latest/topics/cdh_ig_hive_metastore_configure.html#topic_18_4_2
I don't know if there is a clever way to test with different DB/drivers, other 
people may know better.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68767/#review208821
---


On Sept. 24, 2018, 12:16 p.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 24, 2018, 12:16 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68664: HIVE-20306: Implement projection spec for fetching only requested fields from partitions

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68664/#review208866
---



Basically this looks good. All my issues are pretty picky.


standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsResponse.java
Lines 38 (patched)


Why is this new interface marked Stable? I know this is generated code, but 
where does ths come from?



standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
Lines 1664 (patched)


nit: is a *list* of dot separated strings



standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
Lines 1704 (patched)


Maybe add: must be present but is ignored



standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
Lines 2389 (patched)


this is get_partitions with filter and projectspec and unimplemented filter 
spec?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDirectSqlUtils.java
Lines 49 (patched)


add class comment



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PartitionProjectionEvaluator.java
Lines 57 (patched)


Add class description



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PartitionProjectionEvaluator.java
Lines 111 (patched)


is used *to* parse



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PartitionProjectionEvaluator.java
Lines 459 (patched)


use hive coding convention, if () {



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
Lines 1078 (patched)


add method description



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
Lines 1101 (patched)


add method description



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
Lines 1198 (patched)


Is it clearer to combine this bit of code with the similar calculation on 
line 120?



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
Lines 65 (patched)


Add a brief comment explaining what the test does



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
Lines 84 (patched)


This seems weird



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
Lines 132 (patched)


nit: ask intellij to reformat this



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
Lines 161 (patched)


nit: OK you can hate me but I think builder methods should line up like
 return new PartitionBuilder()
.inTable(table)
.setValues(vals)
.addPartParam("key1", "S1")
.addPartParam("key2", "S2")
.addPartParam(EXCLUDE_KEY_PREFIX + "key1", "e1")
.addPartParam(EXCLUDE_KEY_PREFIX + "key2", "e2")
.setBucketCols(table.getSd().getBucketCols())
.setSortCols(table.getSd().getSortCols())
.setSerdeName(table.getSd().getSerdeInfo().getName())
.setSerdeLib(table.getSd().getSerdeInfo().getSerializationLib())
.setSerdeParams(table.getSd().getSerdeInfo().getParameters())
.build(conf);



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
Lines 207 (patched)


use assertEquals
And do you want to use static import for assert statements? I think it 
looks neater.



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java
Lines 234 (patched)

Re: Review Request 68710: HIVE-20544: TOpenSessionReq logs password and username

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68710/#review208862
---




service-rpc/src/gen/thrift/gen-javabean/org/apache/hive/service/rpc/thrift/TOpenSessionReq.java
Line 546 (original), 546 (patched)


why give a clue about password length? Maybe just always print  or 
something?


- Andrew Sherman


On Sept. 21, 2018, 3:31 p.m., Karen Coppage wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68710/
> ---
> 
> (Updated Sept. 21, 2018, 3:31 p.m.)
> 
> 
> Review request for hive and Laszlo Pinter.
> 
> 
> Bugs: HIVE-20544
> https://issues.apache.org/jira/browse/HIVE-20544
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> TOpenSessionReq, if client protocol is unset, both username and password are 
> logged. Logging a password is a security risk. This patch would hide it with 
> asterisks.
> 
> 
> Diffs
> -
> 
>   service-rpc/pom.xml d6a07a55bc 
>   
> service-rpc/src/gen/thrift/gen-javabean/org/apache/hive/service/rpc/thrift/TOpenSessionReq.java
>  3195e704f3 
> 
> 
> Diff: https://reviews.apache.org/r/68710/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Karen Coppage
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Lines 316 (patched)
> > 
> >
> > 1) Does setObject() work OK on all the jdbc drivers that are supported? 
> > In the oast I have seen cases where it was necessary to dispatch to the 
> > correct method like setString, setInt 
> > 2) can the params over be null? Do we need to call setNull instead of 
> > setObject()? Again we need to consider all the drivers.
> 
> Laszlo Pinter wrote:
> 1) The jdbc driver will do the type checking. A slight disadvantage is 
> the minor overhead, but this is negligible as compared to the better 
> maitainable code you end up with.
> 2) You're correct, I have to make sure that the params[i] is not null or 
> use setNull instead.
> 
> Laszlo Pinter wrote:
> So I did a bit more of a debugging, and my previous comment about the 
> params[i] can be null is not correct. The params can contain partitionIds, 
> storageDescriptorIds, columnDescriptorIds, serdeIds, depeding from where the 
> executeNoResult() is called.  These fields are mandatory and cannot be null. 
> If any of these items are null, means that the metastore db is not consistent 
> and it was corrupted.

I worte this once, but rb ate it, sorry if it duplicates.
On 1) Did you test with all drivers?
On 2) I suggest you add some checking to nail down that aprams are non-null. 
How is the java testing of this class? Do we need negative test cases?


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68767/#review208821
---


On Sept. 21, 2018, 3:21 p.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 21, 2018, 3:21 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

> On Sept. 21, 2018, 12:04 a.m., denys kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Line 311 (original), 311 (patched)
> > 
> >
> > Is it a good idea to use PreparedStatement for this particular 
> > use-case? 
> > Prepared statement must be recompiled each time you call it with a 
> > different number of parameters. Aside from the added time from recompiling 
> > the prepared statement, other queries may be removed from the pool of 
> > available statements causing them to be recompiled again too.

Just to jump in, I think the benefits of using PreparedStatements everywhere 
outweigh the costs.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68767/#review208827
---


On Sept. 21, 2018, 3:21 p.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 21, 2018, 3:21 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> >

This all looks good, I haver questions...


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68767/#review208821
---


On Sept. 19, 2018, 9:46 a.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 19, 2018, 9:46 a.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68767/#review208821
---




standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
Line 316 (original), 315 (patched)


may be neater to use 'for (String param: params)' or whatever the syntax is



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
Lines 316 (patched)


1) Does setObject() work OK on all the jdbc drivers that are supported? In 
the oast I have seen cases where it was necessary to dispatch to the correct 
method like setString, setInt 
2) can the params over be null? Do we need to call setNull instead of 
setObject()? Again we need to consider all the drivers.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
Line 317 (original), 320 (patched)


I note the tracing will be less intersting now. Do we now need to insert 
the paramters as well?


- Andrew Sherman


On Sept. 19, 2018, 9:46 a.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 19, 2018, 9:46 a.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68365: HIVE-19253: HMS ignores tableType property for external tables

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68365/#review207383
---


Ship it!




Ship It!

- Andrew Sherman


On Aug. 15, 2018, 11:13 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68365/
> ---
> 
> (Updated Aug. 15, 2018, 11:13 p.m.)
> 
> 
> Review request for hive, Andrew Sherman, Peter Vary, and Vihang Karajgaonkar.
> 
> 
> Bugs: HIVE-19253
> https://issues.apache.org/jira/browse/HIVE-19253
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-19253: HMS ignores tableType property for external tables
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  8e2f94eb691a72cc4e6410aa76b176032d4af673 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  5233bee59220244e89f05b6c4dbf86a2cc6dc9fe 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  b74c3048fa2e18adc7f0d7cc813a180d4466fa36 
> 
> 
> Diff: https://reviews.apache.org/r/68365/diff/2/
> 
> 
> Testing
> ---
> 
> Added unit test to verify the expected behavior.
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 68365: HIVE-19253: HMS ignores tableType property for external tables

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68365/#review207354
---




standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 839 (patched)


It seems weird that there isn't a constant for "EXTERNAL", but I can't find 
one.



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 854 (patched)


So you test 
params(EXTERNAL=true),TableType.MANAGED_TABLE
params(EXTERNAL=false),TableType.EXTERNAL_TABLE
having done all the work you may as well test
params(EXTERNAL=false),TableType.MANAGED_TABLE
params(EXTERNAL=true),TableType.EXTERNAL_TABLE
as well?


- Andrew Sherman


On Aug. 15, 2018, 8:18 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68365/
> ---
> 
> (Updated Aug. 15, 2018, 8:18 p.m.)
> 
> 
> Review request for hive, Andrew Sherman, Peter Vary, and Vihang Karajgaonkar.
> 
> 
> Bugs: HIVE-19253
> https://issues.apache.org/jira/browse/HIVE-19253
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-19253: HMS ignores tableType property for external tables
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  8e2f94eb691a72cc4e6410aa76b176032d4af673 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  b74c3048fa2e18adc7f0d7cc813a180d4466fa36 
> 
> 
> Diff: https://reviews.apache.org/r/68365/diff/1/
> 
> 
> Testing
> ---
> 
> Added unit test to verify the expected behavior.
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 68027: HIVE-19986: Add logging of runtime statistics indicating when Hdfs Erasure Coding is used by MR. These stats are not avalable until the unreleased Hadoop 3.2 so the shim is u

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68027/
---

(Updated July 24, 2018, 9:15 p.m.)


Review request for hive and Sahil Takiar.


Bugs: HIVE-19986
https://issues.apache.org/jira/browse/HIVE-19986


Repository: hive-git


Description
---

HIVE-19986: Add logging of runtime statistics indicating when Hdfs Erasure 
Coding is used by MR. These stats are not avalable until the unreleased Hadoop 
3.2 so the shim is used to determine if the new counter can be used.


Diffs (updated)
-

  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2ErasureCoding.java
 b0a0145a4ee705b0a7d8f214d2c87397f731faec 
  ql/src/java/org/apache/hadoop/hive/ql/MapRedStats.java 
483c3d9fcd2f55a644321a62224b13846e421188 
  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/HadoopJobExecHelper.java 
c31e22f041e18abf077f44e79188b7479abc3629 
  ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
46114f50dc1ee3f03305cf9047361c39131128d6 
  shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
02490f1171ee658a2f3208e4cf2f0416d6907dc6 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
2e84ca9b180e0149fd2185183238dab2268e5474 


Diff: https://reviews.apache.org/r/68027/diff/2/

Changes: https://reviews.apache.org/r/68027/diff/1-2/


Testing
---

New test added to TestJdbcWithMiniHS2ErasureCoding.java.
I also ran this code against a hadoop 3.2 snapshot to ensure the new stat works


Thanks,

Andrew Sherman

Re: Review Request 67468: HIVE-18118: provide supportability support for Erasure Coding Update number of Erasure Coded Files in a directory as part of Basic (aka Quick) Stats This information is then

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67468/
---

(Updated June 21, 2018, 4:32 p.m.)


Review request for hive and Sahil Takiar.


Summary (updated)
-

HIVE-18118: provide supportability support for Erasure Coding Update number of 
Erasure Coded Files in a directory as part of Basic (aka Quick) Stats This 
information is then (mostly) available through 'EXPLAIN EXTENDED' and 'DESCRIBE 
EXTENDED' Extend the MiniHS2 Builder to allow configuring the numb


Repository: hive-git


Description
---

HIVE-18118: provide supportability support for Erasure Coding Update number of 
Erasure Coded Files in a directory as part of Basic (aka Quick) Stats This 
information is then (mostly) available through 'EXPLAIN EXTENDED' and 'DESCRIBE 
EXTENDED' Extend the MiniHS2 Builder to allow configuring the number of 
datanodes. Add a jdbc MiniHS2/Spark test that uses Erasure Coding. There are 
some change to StatsSetupConst to make checkstyle happy.


Diffs (updated)
-

  common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
cfe9b2208a60586a05d293f222aa90b37e9a06ac 
  itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
d7d7097336fc6be4c2f7a35cd6897e0375486e81 
  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2ErasureCoding.java
 PRE-CREATION 
  itests/src/test/resources/testconfiguration.properties 
aeb6211f5a11f6b9466d731cccb3e55cb03281cb 
  itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
2106fec7af75958644eb831498b725b771ddf47a 
  itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
1700c08d3f37285de43b5d4fe5c77ef55c170235 
  ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
8e32b02b59c4e36e0dd610beb6aacf80c3ac555d 
  
ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 
58c8960c096f8885086be4f46dc1e33edd26249a 
  
ql/src/java/org/apache/hadoop/hive/ql/metadata/formatting/MetaDataFormatUtils.java
 44687ef471f76bb6c8baee3c9081a191e2d0e74d 
  
ql/src/java/org/apache/hadoop/hive/ql/metadata/formatting/TextMetaDataFormatter.java
 326cbedcf0194bfa42b66557fc88f6285df1c619 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/spark/SparkMapJoinOptimizer.java
 8e75db9e08d575eb8ba7123251eaca9e2097a7af 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java
 d0be33bd0fd83c829584f069a12e36b278e4d6b2 
  ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java 
61458b4e256f7bf63f781a3135059257a2b8ddd4 
  ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java 
2c5b6557ce6462151e764c17064354f448ee708d 
  ql/src/java/org/apache/hadoop/hive/ql/plan/Statistics.java 
fd461ae52930de54d993f0df74c0635f82fcc799 
  ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java 
4068e5670fe8a65593228d2e3df9d809e836a696 
  ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsNoJobTask.java 
d4d46a3671efdaaed32f63b7262b963cce00b94e 
  ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java 
8c238871765b0d5312a459a0e7f68c81f3837c13 
  ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java 
e00098529f78aa4950c8cec301b966999ae9bf96 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 
60447192b11261727a63219fb2e69f09fd425aa0 
  ql/src/test/queries/clientpositive/erasure_explain.q PRE-CREATION 
  ql/src/test/queries/clientpositive/erasure_simple.q 
c08409c17787417b986d90a43104f5ddd456e600 
  ql/src/test/results/clientpositive/erasurecoding/erasure_explain.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
01f6015a346c1e4283fd6a8cf1eaa3b670450e20 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/common/StatsSetupConst.java
 78ea01d9687fe043d63441430c46b30c25cd9756 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 e88f9a5fee4b2cbd99ec7c5c5350f8c2b8015384 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
 16a8c758010c1f81e07296362157bb24260bcf3f 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreUtils.java
 55ff1502d415dea52095cfdd523d01f1e49ce084 


Diff: https://reviews.apache.org/r/67468/diff/3/

Changes: https://reviews.apache.org/r/67468/diff/2-3/


Testing
---

Ran driver tests and new jdbc test


Thanks,

Andrew Sherman

Re: Review Request 67468: FIXES TO MAKE INTELLIJ HAPPY DO NOT PUSH

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67468/
---

(Updated June 21, 2018, 12:33 a.m.)


Review request for hive and Sahil Takiar.


Summary (updated)
-

FIXES TO MAKE INTELLIJ HAPPY DO NOT PUSH


Repository: hive-git


Description (updated)
---

HIVE-18118: provide supportability support for Erasure Coding Update number of 
Erasure Coded Files in a directory as part of Basic (aka Quick) Stats This 
information is then (mostly) available through 'EXPLAIN EXTENDED' and 'DESCRIBE 
EXTENDED' Extend the MiniHS2 Builder to allow configuring the number of 
datanodes. Add a jdbc MiniHS2/Spark test that uses Erasure Coding. There are 
some change to StatsSetupConst to make checkstyle happy.


Diffs (updated)
-

  common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
cfe9b2208a60586a05d293f222aa90b37e9a06ac 
  itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
d7d7097336fc6be4c2f7a35cd6897e0375486e81 
  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2ErasureCoding.java
 PRE-CREATION 
  itests/src/test/resources/testconfiguration.properties 
aeb6211f5a11f6b9466d731cccb3e55cb03281cb 
  itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
2106fec7af75958644eb831498b725b771ddf47a 
  itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
1700c08d3f37285de43b5d4fe5c77ef55c170235 
  ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
8e32b02b59c4e36e0dd610beb6aacf80c3ac555d 
  ql/src/java/org/apache/hadoop/hive/ql/exec/OperatorUtils.java 
7b2ae40107c21fb2894f078fe4996df16b2977c2 
  
ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 
58c8960c096f8885086be4f46dc1e33edd26249a 
  
ql/src/java/org/apache/hadoop/hive/ql/metadata/formatting/MetaDataFormatUtils.java
 44687ef471f76bb6c8baee3c9081a191e2d0e74d 
  
ql/src/java/org/apache/hadoop/hive/ql/metadata/formatting/TextMetaDataFormatter.java
 326cbedcf0194bfa42b66557fc88f6285df1c619 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/QueryPlanPostProcessor.java 
a91f45e204b1f37896a8c561c11df4b6e62b4192 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/spark/SparkMapJoinOptimizer.java
 8e75db9e08d575eb8ba7123251eaca9e2097a7af 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java
 d0be33bd0fd83c829584f069a12e36b278e4d6b2 
  ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java 
596eddedf5a1c86007b01d71e82c01f0591c3d37 
  ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java 
61458b4e256f7bf63f781a3135059257a2b8ddd4 
  ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java 
2c5b6557ce6462151e764c17064354f448ee708d 
  ql/src/java/org/apache/hadoop/hive/ql/plan/Statistics.java 
fd461ae52930de54d993f0df74c0635f82fcc799 
  ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java 
4068e5670fe8a65593228d2e3df9d809e836a696 
  ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsNoJobTask.java 
d4d46a3671efdaaed32f63b7262b963cce00b94e 
  ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java 
8c238871765b0d5312a459a0e7f68c81f3837c13 
  ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java 
e00098529f78aa4950c8cec301b966999ae9bf96 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 
60447192b11261727a63219fb2e69f09fd425aa0 
  ql/src/test/queries/clientpositive/erasure_explain.q PRE-CREATION 
  ql/src/test/queries/clientpositive/erasure_simple.q 
c08409c17787417b986d90a43104f5ddd456e600 
  ql/src/test/results/clientpositive/erasurecoding/erasure_explain.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
01f6015a346c1e4283fd6a8cf1eaa3b670450e20 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/common/StatsSetupConst.java
 78ea01d9687fe043d63441430c46b30c25cd9756 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 e88f9a5fee4b2cbd99ec7c5c5350f8c2b8015384 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
 16a8c758010c1f81e07296362157bb24260bcf3f 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreUtils.java
 55ff1502d415dea52095cfdd523d01f1e49ce084 


Diff: https://reviews.apache.org/r/67468/diff/2/

Changes: https://reviews.apache.org/r/67468/diff/1-2/


Testing
---

Ran driver tests and new jdbc test


Thanks,

Andrew Sherman

Re: Review Request 67468: HIVE-18118: provide supportability support for Erasure Coding Update number of Erasure Coded Files in a directory as part of Basic (aka Quick) Stats This information is then

[Andrew Sherman via Review Board]

> On June 15, 2018, 1:37 p.m., Sahil Takiar wrote:
> > Why not show the #of EC files for regular explain plans too? To decrease 
> > the # of q file updates, it can be omitted if the # of EC files = 0

I saw that regular explain did not report numFiles so I did not report 
numEcFiles there.
I think you are saying that IF EC files are in a dir then instead of 
"Statistics: Num rows: 1 Data size: 15812" I would print  "Statistics: Num 
rows: 1 Data size: 15812 Erasure files: 2". Is that right?
Are you also suggesting that in extended explain I should also not report 
erasure coded file count unless it is positive? (This would require some small 
code changes as we currently I think just rely on dumping the properties )


> On June 15, 2018, 1:37 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/common/StatsSetupConst.java
> > Line 116 (original), 122 (patched)
> > 
> >
> > why change this from an array to a list?

When I changed some of the arrays to have new members, checkstyle didn't like 
it and suggested Lists. It was ugly to have some Lists and some arrays so I 
changed all the similar ones to be Lists.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67468/#review204836
---


On June 6, 2018, 12:46 a.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67468/
> ---
> 
> (Updated June 6, 2018, 12:46 a.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-18118: provide supportability support for Erasure Coding 
> [NOTE THIS REVIEW INITIALLY OMITS 200+ .q.out changes]
> Update number of Erasure Coded Files in a directory as part of Basic (aka 
> Quick) Stats 
> This information is then (mostly) available through 'EXPLAIN EXTENDED' and 
> 'DESCRIBE EXTENDED' 
> Extend the MiniHS2 Builder to allow configuring the number of datanodes. 
> Add a jdbc MiniHS2/Spark test that uses Erasure Coding. 
> There are some change to StatsSetupConst to make checkstyle happy.
> 
> 
> Diffs
> -
> 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
> d7d7097336fc6be4c2f7a35cd6897e0375486e81 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2ErasureCoding.java
>  PRE-CREATION 
>   itests/src/test/resources/testconfiguration.properties 
> 463fda1913f6d5b928fcee038f19e124b0239e96 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
> 2365fb76bd08f3a310e81ac3a19ca64971aeec8e 
>   itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
> 1700c08d3f37285de43b5d4fe5c77ef55c170235 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
> e06949928d179cfd9a4dcb7176203b885509 
>   
> ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java
>  209fdfb287cabc5bb7cab2117d771f7907deb2b9 
>   ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsNoJobTask.java 
> d4d46a3671efdaaed32f63b7262b963cce00b94e 
>   ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java 
> 8c238871765b0d5312a459a0e7f68c81f3837c13 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 
> 982b18076180ba300094f30a7f87f025f993b265 
>   ql/src/test/queries/clientpositive/erasure_explain.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/erasure_simple.q 
> c08409c17787417b986d90a43104f5ddd456e600 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_explain.q.out 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
> 01f6015a346c1e4283fd6a8cf1eaa3b670450e20 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/common/StatsSetupConst.java
>  78ea01d9687fe043d63441430c46b30c25cd9756 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  77ed2b4de4569fa8aca23b16f2b362b187c7c4fc 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  9b36d09eb9fb332e913d442bb476628eca334b6e 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreUtils.java
>  55ff1502d415dea52095cfdd523d01f1e49ce084 
> 
> 
> Diff: https://reviews.apache.org/r/67468/diff/1/
> 
> 
> Testing
> ---
> 
> Ran driver tests and new jdbc test
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Review Request 67468: HIVE-18118: provide supportability support for Erasure Coding Update number of Erasure Coded Files in a directory as part of Basic (aka Quick) Stats This information is then (mos

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67468/
---

Review request for hive and Sahil Takiar.


Repository: hive-git


Description
---

HIVE-18118: provide supportability support for Erasure Coding 
[NOTE THIS REVIEW INITIALLY OMITS 200+ .q.out changes]
Update number of Erasure Coded Files in a directory as part of Basic (aka 
Quick) Stats 
This information is then (mostly) available through 'EXPLAIN EXTENDED' and 
'DESCRIBE EXTENDED' 
Extend the MiniHS2 Builder to allow configuring the number of datanodes. 
Add a jdbc MiniHS2/Spark test that uses Erasure Coding. 
There are some change to StatsSetupConst to make checkstyle happy.


Diffs
-

  itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
d7d7097336fc6be4c2f7a35cd6897e0375486e81 
  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2ErasureCoding.java
 PRE-CREATION 
  itests/src/test/resources/testconfiguration.properties 
463fda1913f6d5b928fcee038f19e124b0239e96 
  itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
2365fb76bd08f3a310e81ac3a19ca64971aeec8e 
  itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
1700c08d3f37285de43b5d4fe5c77ef55c170235 
  ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
e06949928d179cfd9a4dcb7176203b885509 
  
ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 
209fdfb287cabc5bb7cab2117d771f7907deb2b9 
  ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsNoJobTask.java 
d4d46a3671efdaaed32f63b7262b963cce00b94e 
  ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java 
8c238871765b0d5312a459a0e7f68c81f3837c13 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 
982b18076180ba300094f30a7f87f025f993b265 
  ql/src/test/queries/clientpositive/erasure_explain.q PRE-CREATION 
  ql/src/test/queries/clientpositive/erasure_simple.q 
c08409c17787417b986d90a43104f5ddd456e600 
  ql/src/test/results/clientpositive/erasurecoding/erasure_explain.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
01f6015a346c1e4283fd6a8cf1eaa3b670450e20 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/common/StatsSetupConst.java
 78ea01d9687fe043d63441430c46b30c25cd9756 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 77ed2b4de4569fa8aca23b16f2b362b187c7c4fc 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
 9b36d09eb9fb332e913d442bb476628eca334b6e 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreUtils.java
 55ff1502d415dea52095cfdd523d01f1e49ce084 


Diff: https://reviews.apache.org/r/67468/diff/1/


Testing
---

Ran driver tests and new jdbc test


Thanks,

Andrew Sherman

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.

[Andrew Sherman via Review Board]

> On May 10, 2018, noon, Sahil Takiar wrote:
> > testutils/ptest2/conf/deployed/master-mr2.properties
> > Line 75 (original), 75 (patched)
> > 
> >
> > have you manually deployed these changes to the ptest server? this file 
> > is just a copy of whats already been deployed, so its just for reference
> > 
> > also, why skip batching?
> 
> Andrew Sherman wrote:
> OK I have no idea what ut.itests.qtest.skipBatching means I just copied 
> TestEncryptedHDFSCliDriver :-(
> 
> As for deploying these changes, I don't know what that means, my new 
> tests did appear to run. can you explain more?
> 
> Sahil Takiar wrote:
> anytime you add a new cli driver, you have to manually modify a file on 
> the ptest master server, you have to modify the file 
> `/usr/local/hiveptest/profiles/master-mr2.properties` you probably don't have 
> permissions though, so let me know the final diff for the this file and I can 
> deploy it.
> 
> can we check if batching can be used for this? i think batching means 
> that q-tests get bundled together into a "batch" of q-tests that are run in a 
> single `mvn test` command
> 
> Andrew Sherman wrote:
> Batching works locally for me so I have removed this. I'll provide a 
> patch for master-mr2.properties before commit

The weird thing is that the new driver does get run already, and my two new 
tests run: 
https://builds.apache.org/job/PreCommit-HIVE-Build/3/testReport/org.apache.hadoop.hive.cli/TestErasureCodingHDFSCliDriver/
Even so, please update the master-mr2.properties file, maybe there is some 
sublety. The diff in 
https://issues.apache.org/jira/secure/attachment/12924372/HIVE-18117.9.patch#file-16
 is good.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/#review202831
---


On May 14, 2018, 9:44 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67023/
> ---
> 
> (Updated May 14, 2018, 9:44 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor 
> "ErasureProcessor"
> which allows .q files to contain Erasure Coding commands similar to those 
> provided
> by the hdfs ec command
> https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
> The Erasure Coding functionality is exposed through a new shim 
> "HdfsFileErasureCodingPolicy".
> At this stage there are two .q files:
> erasure_commnds.q (a simple test to show ERASURE commands can run on local fs 
> via
> TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
> erasure_simple.q (which does some trivial queries to demonstrate basic 
> functionality).
> More tests will come in future commits.
> 
> 
> Diffs
> -
> 
>   
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java
>  PRE-CREATION 
>   itests/src/test/resources/testconfiguration.properties 
> cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 
> 1814f0fa190e0ebcf327aebcdaf6f9967a5fd14f 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
> 16571b3ff3288dfb99fbca570452592cc1650f9a 
>   
> ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 
> 3d47991b603c94c8da2106e67192c8513ef783a7 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 
> 56c7516ecfaf2421b0f3d3a188d05f38715b25b2 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> 89129f99fe63f0384aff965ad665770d11e9af04 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  de43c2866f64e2ed5c74eab450de28f1a79248dc 
>   ql/src/test/queries/clientpositive/erasure_commands.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/erasure_simple.q PRE-CREATION 
>   ql/src/test/results/clientpositive/erasure_commands.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_commands.q.out 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
> PRE-CREATION 
>   shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
> ec06a88dc21346473bec6589c703167d50e3b367 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
> b89081761780bf1f305d0196bb94bb0b54f7184f 
>   testutils/ptest2/conf/deployed/master-mr2.properties 
> 

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.

[Andrew Sherman via Review Board]

> On May 10, 2018, noon, Sahil Takiar wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java
> > Lines 674 (patched)
> > 
> >
> > is this necessary since you set the cluster type to mr above?
> 
> Andrew Sherman wrote:
> Ha good question. Yes it is necessary as setClusterType() does not always 
> set the cluster type :-( - it allows the cluster type to overridden with 
> -Dclustermode=xxx
> 
> Sahil Takiar wrote:
> interesting, should we handle other cluster types like Spark or MR too?
> 
> Andrew Sherman wrote:
> looks like it does already
> 
> Sahil Takiar wrote:
> then is the tez specific code necessary?
> 
> Andrew Sherman wrote:
> I think it is, as tez has its own hive-site.xml but I am not 100% sure
> 
> Sahil Takiar wrote:
> yes, tez has its own `hive-site.xml`, as does Spark; so do we need to do 
> this for Spark too?

That would make sense but then we have to decide if that is 
data/conf/spark/local or data/conf/spark/local :-(
These code paths are confusing becaus it is hard to anticipate how they'll be 
used.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/#review202831
---


On May 14, 2018, 9:44 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67023/
> ---
> 
> (Updated May 14, 2018, 9:44 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor 
> "ErasureProcessor"
> which allows .q files to contain Erasure Coding commands similar to those 
> provided
> by the hdfs ec command
> https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
> The Erasure Coding functionality is exposed through a new shim 
> "HdfsFileErasureCodingPolicy".
> At this stage there are two .q files:
> erasure_commnds.q (a simple test to show ERASURE commands can run on local fs 
> via
> TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
> erasure_simple.q (which does some trivial queries to demonstrate basic 
> functionality).
> More tests will come in future commits.
> 
> 
> Diffs
> -
> 
>   
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java
>  PRE-CREATION 
>   itests/src/test/resources/testconfiguration.properties 
> cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 
> 1814f0fa190e0ebcf327aebcdaf6f9967a5fd14f 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
> 16571b3ff3288dfb99fbca570452592cc1650f9a 
>   
> ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 
> 3d47991b603c94c8da2106e67192c8513ef783a7 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 
> 56c7516ecfaf2421b0f3d3a188d05f38715b25b2 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> 89129f99fe63f0384aff965ad665770d11e9af04 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  de43c2866f64e2ed5c74eab450de28f1a79248dc 
>   ql/src/test/queries/clientpositive/erasure_commands.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/erasure_simple.q PRE-CREATION 
>   ql/src/test/results/clientpositive/erasure_commands.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_commands.q.out 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
> PRE-CREATION 
>   shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
> ec06a88dc21346473bec6589c703167d50e3b367 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
> b89081761780bf1f305d0196bb94bb0b54f7184f 
>   testutils/ptest2/conf/deployed/master-mr2.properties 
> 7edc307f85744d60d322ad8087164625677fc230 
> 
> 
> Diff: https://reviews.apache.org/r/67023/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.

[Andrew Sherman via Review Board]

> On May 14, 2018, 4:57 p.m., Sahil Takiar wrote:
> > itests/src/test/resources/testconfiguration.properties
> > Line 1693 (original), 1693 (patched)
> > 
> >
> > why would we want the ec commands to work outside the 
> > `TestErasureCodingHDFSCliDriver`?
> 
> Andrew Sherman wrote:
> So you could run an ec test in TestCliDriver (sorry reviewboard lost my 
> ealrier reply)
> 
> Sahil Takiar wrote:
> why would you want to do that? whats the use case? shouldn't 
> `TestErasureCodingHDFSCliDriver` encapsulate all EC-related tests? plus I'm 
> not sure how you could run any EC commands against a local filesystem, 
> wouldn't they all be no-ops?

They would. When I'm developing an EC test I may sometimes want to run the test 
in TestCLiDriver with erasure commands being no-ops as a way to validate the 
script. Myabe this is weird, but I've been doing it.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/#review203040
---


On May 14, 2018, 9:44 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67023/
> ---
> 
> (Updated May 14, 2018, 9:44 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor 
> "ErasureProcessor"
> which allows .q files to contain Erasure Coding commands similar to those 
> provided
> by the hdfs ec command
> https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
> The Erasure Coding functionality is exposed through a new shim 
> "HdfsFileErasureCodingPolicy".
> At this stage there are two .q files:
> erasure_commnds.q (a simple test to show ERASURE commands can run on local fs 
> via
> TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
> erasure_simple.q (which does some trivial queries to demonstrate basic 
> functionality).
> More tests will come in future commits.
> 
> 
> Diffs
> -
> 
>   
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java
>  PRE-CREATION 
>   itests/src/test/resources/testconfiguration.properties 
> cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 
> 1814f0fa190e0ebcf327aebcdaf6f9967a5fd14f 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
> 16571b3ff3288dfb99fbca570452592cc1650f9a 
>   
> ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 
> 3d47991b603c94c8da2106e67192c8513ef783a7 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 
> 56c7516ecfaf2421b0f3d3a188d05f38715b25b2 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> 89129f99fe63f0384aff965ad665770d11e9af04 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  de43c2866f64e2ed5c74eab450de28f1a79248dc 
>   ql/src/test/queries/clientpositive/erasure_commands.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/erasure_simple.q PRE-CREATION 
>   ql/src/test/results/clientpositive/erasure_commands.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_commands.q.out 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
> PRE-CREATION 
>   shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
> ec06a88dc21346473bec6589c703167d50e3b367 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
> b89081761780bf1f305d0196bb94bb0b54f7184f 
>   testutils/ptest2/conf/deployed/master-mr2.properties 
> 7edc307f85744d60d322ad8087164625677fc230 
> 
> 
> Diff: https://reviews.apache.org/r/67023/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.

[Andrew Sherman via Review Board]

> On May 10, 2018, noon, Sahil Takiar wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java
> > Lines 674 (patched)
> > 
> >
> > is this necessary since you set the cluster type to mr above?
> 
> Andrew Sherman wrote:
> Ha good question. Yes it is necessary as setClusterType() does not always 
> set the cluster type :-( - it allows the cluster type to overridden with 
> -Dclustermode=xxx
> 
> Sahil Takiar wrote:
> interesting, should we handle other cluster types like Spark or MR too?
> 
> Andrew Sherman wrote:
> looks like it does already
> 
> Sahil Takiar wrote:
> then is the tez specific code necessary?

I think it is, as tez has its own hive-site.xml but I am not 100% sure


> On May 10, 2018, noon, Sahil Takiar wrote:
> > testutils/ptest2/conf/deployed/master-mr2.properties
> > Line 75 (original), 75 (patched)
> > 
> >
> > have you manually deployed these changes to the ptest server? this file 
> > is just a copy of whats already been deployed, so its just for reference
> > 
> > also, why skip batching?
> 
> Andrew Sherman wrote:
> OK I have no idea what ut.itests.qtest.skipBatching means I just copied 
> TestEncryptedHDFSCliDriver :-(
> 
> As for deploying these changes, I don't know what that means, my new 
> tests did appear to run. can you explain more?
> 
> Sahil Takiar wrote:
> anytime you add a new cli driver, you have to manually modify a file on 
> the ptest master server, you have to modify the file 
> `/usr/local/hiveptest/profiles/master-mr2.properties` you probably don't have 
> permissions though, so let me know the final diff for the this file and I can 
> deploy it.
> 
> can we check if batching can be used for this? i think batching means 
> that q-tests get bundled together into a "batch" of q-tests that are run in a 
> single `mvn test` command

Batching works locally for me so I have removed this. I'll provide a patch for 
master-mr2.properties before commit


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/#review202831
---


On May 14, 2018, 9:44 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67023/
> ---
> 
> (Updated May 14, 2018, 9:44 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor 
> "ErasureProcessor"
> which allows .q files to contain Erasure Coding commands similar to those 
> provided
> by the hdfs ec command
> https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
> The Erasure Coding functionality is exposed through a new shim 
> "HdfsFileErasureCodingPolicy".
> At this stage there are two .q files:
> erasure_commnds.q (a simple test to show ERASURE commands can run on local fs 
> via
> TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
> erasure_simple.q (which does some trivial queries to demonstrate basic 
> functionality).
> More tests will come in future commits.
> 
> 
> Diffs
> -
> 
>   
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java
>  PRE-CREATION 
>   itests/src/test/resources/testconfiguration.properties 
> cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 
> 1814f0fa190e0ebcf327aebcdaf6f9967a5fd14f 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
> 16571b3ff3288dfb99fbca570452592cc1650f9a 
>   
> ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 
> 3d47991b603c94c8da2106e67192c8513ef783a7 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 
> 56c7516ecfaf2421b0f3d3a188d05f38715b25b2 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> 89129f99fe63f0384aff965ad665770d11e9af04 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  de43c2866f64e2ed5c74eab450de28f1a79248dc 
>   ql/src/test/queries/clientpositive/erasure_commands.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/erasure_simple.q PRE-CREATION 
>   ql/src/test/results/clientpositive/erasure_commands.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_commands.q.out 
> PRE-CREATION 
>   

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/
---

(Updated May 14, 2018, 9:44 p.m.)


Review request for hive and Sahil Takiar.


Repository: hive-git


Description
---

TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor 
"ErasureProcessor"
which allows .q files to contain Erasure Coding commands similar to those 
provided
by the hdfs ec command
https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
The Erasure Coding functionality is exposed through a new shim 
"HdfsFileErasureCodingPolicy".
At this stage there are two .q files:
erasure_commnds.q (a simple test to show ERASURE commands can run on local fs 
via
TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
erasure_simple.q (which does some trivial queries to demonstrate basic 
functionality).
More tests will come in future commits.


Diffs (updated)
-

  
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java
 PRE-CREATION 
  itests/src/test/resources/testconfiguration.properties 
cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
  itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 
1814f0fa190e0ebcf327aebcdaf6f9967a5fd14f 
  itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
16571b3ff3288dfb99fbca570452592cc1650f9a 
  ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 
3d47991b603c94c8da2106e67192c8513ef783a7 
  ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 
56c7516ecfaf2421b0f3d3a188d05f38715b25b2 
  ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
89129f99fe63f0384aff965ad665770d11e9af04 
  
ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
 de43c2866f64e2ed5c74eab450de28f1a79248dc 
  ql/src/test/queries/clientpositive/erasure_commands.q PRE-CREATION 
  ql/src/test/queries/clientpositive/erasure_simple.q PRE-CREATION 
  ql/src/test/results/clientpositive/erasure_commands.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/erasurecoding/erasure_commands.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
PRE-CREATION 
  shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
ec06a88dc21346473bec6589c703167d50e3b367 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
b89081761780bf1f305d0196bb94bb0b54f7184f 
  testutils/ptest2/conf/deployed/master-mr2.properties 
7edc307f85744d60d322ad8087164625677fc230 


Diff: https://reviews.apache.org/r/67023/diff/3/

Changes: https://reviews.apache.org/r/67023/diff/2-3/


Testing
---


Thanks,

Andrew Sherman

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.

[Andrew Sherman via Review Board]

> On May 14, 2018, 4:57 p.m., Sahil Takiar wrote:
> > itests/src/test/resources/testconfiguration.properties
> > Line 1693 (original), 1693 (patched)
> > 
> >
> > why would we want the ec commands to work outside the 
> > `TestErasureCodingHDFSCliDriver`?

So you could run an ec test in TestCliDriver (sorry reviewboard lost my ealrier 
reply)


> On May 14, 2018, 4:57 p.m., Sahil Takiar wrote:
> > ql/src/test/queries/clientpositive/erasure_commands.q
> > Lines 2 (patched)
> > 
> >
> > why would we want to run this on the local fs?

I'm imaginging wanting to run an ec test in TestCliDriver without changing it 
(sorry reviewboard lost my ealrier reply)


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/#review203040
---


On May 11, 2018, 11:38 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67023/
> ---
> 
> (Updated May 11, 2018, 11:38 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor 
> "ErasureProcessor"
> which allows .q files to contain Erasure Coding commands similar to those 
> provided
> by the hdfs ec command
> https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
> The Erasure Coding functionality is exposed through a new shim 
> "HdfsFileErasureCodingPolicy".
> At this stage there are two .q files:
> erasure_commnds.q (a simple test to show ERASURE commands can run on local fs 
> via
> TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
> erasure_simple.q (which does some trivial queries to demonstrate basic 
> functionality).
> More tests will come in future commits.
> 
> 
> Diffs
> -
> 
>   
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java
>  PRE-CREATION 
>   itests/src/test/resources/testconfiguration.properties 
> cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 
> 6628336807b06cab49063673be0d8e9c5b5a7101 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
> 750fc69c5f210ca8f7bfe81b82ee9e3001fc07ba 
>   
> ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 
> 3d47991b603c94c8da2106e67192c8513ef783a7 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 
> 56c7516ecfaf2421b0f3d3a188d05f38715b25b2 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> 9f65a771f95a7c0bd3fdb4e56e47c0fc70235850 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  de43c2866f64e2ed5c74eab450de28f1a79248dc 
>   ql/src/test/queries/clientpositive/erasure_commands.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/erasure_simple.q PRE-CREATION 
>   ql/src/test/results/clientpositive/erasure_commands.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_commands.q.out 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
> PRE-CREATION 
>   shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
> ec06a88dc21346473bec6589c703167d50e3b367 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
> b89081761780bf1f305d0196bb94bb0b54f7184f 
>   testutils/ptest2/conf/deployed/master-mr2.properties 
> 7edc307f85744d60d322ad8087164625677fc230 
> 
> 
> Diff: https://reviews.apache.org/r/67023/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.

[Andrew Sherman via Review Board]

> On May 10, 2018, noon, Sahil Takiar wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java
> > Lines 674 (patched)
> > 
> >
> > is this necessary since you set the cluster type to mr above?
> 
> Andrew Sherman wrote:
> Ha good question. Yes it is necessary as setClusterType() does not always 
> set the cluster type :-( - it allows the cluster type to overridden with 
> -Dclustermode=xxx
> 
> Sahil Takiar wrote:
> interesting, should we handle other cluster types like Spark or MR too?

looks like it does already


> On May 10, 2018, noon, Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
> > Lines 545 (patched)
> > 
> >
> > whats the cache for? can q-tests even specify custom URIs? whats the 
> > use case for support multiple fs URIs?
> 
> Andrew Sherman wrote:
> OK I admit I copied this code from the way hdfsEncryptionShims works 
> without fully understanding it.
> 
> Sahil Takiar wrote:
> can we delete it then? i didn't realize this would require modifying code 
> outside of itests, so I think we should make any changes to core Hive as 
> minimal as possible

deleted


> On May 10, 2018, noon, Sahil Takiar wrote:
> > shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java
> > Lines 699 (patched)
> > 
> >
> > whats the use case for this dummy class? so we can run ec tests on 
> > hadoop versions that don't support ec? wouldn't be just disable the 
> > clidriver entirely for versions that don't support ec?
> 
> Andrew Sherman wrote:
> I'm imagining wanting to run a test on bith EC and non-EC
> 
> Sahil Takiar wrote:
> i thought the `NoopHdfsErasureCodingShim` was used when "the hadoop 
> version does not support hdfs Erasure Coding". u can still run on EC and 
> non-EC folders without this, right?

Yes that is the use of NoopHdfsErasureCodingShim. I think you always need 2 
implementations of the interface


> On May 10, 2018, noon, Sahil Takiar wrote:
> > shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java
> > Lines 741 (patched)
> > 
> >
> > since we have a cache anyway, would it make more sense to just remove 
> > this and make it a loading cache?
> 
> Andrew Sherman wrote:
> I don't know, maybe. There is some advantage in keeping the same 
> structure as the HdfsEncryptionShim.
> 
> Sahil Takiar wrote:
> see comment above about removing the cache

OK I have removed the cache and it does make the code simpler


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/#review202831
---


On May 11, 2018, 11:38 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67023/
> ---
> 
> (Updated May 11, 2018, 11:38 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor 
> "ErasureProcessor"
> which allows .q files to contain Erasure Coding commands similar to those 
> provided
> by the hdfs ec command
> https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
> The Erasure Coding functionality is exposed through a new shim 
> "HdfsFileErasureCodingPolicy".
> At this stage there are two .q files:
> erasure_commnds.q (a simple test to show ERASURE commands can run on local fs 
> via
> TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
> erasure_simple.q (which does some trivial queries to demonstrate basic 
> functionality).
> More tests will come in future commits.
> 
> 
> Diffs
> -
> 
>   
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java
>  PRE-CREATION 
>   itests/src/test/resources/testconfiguration.properties 
> cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 
> 6628336807b06cab49063673be0d8e9c5b5a7101 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
> 750fc69c5f210ca8f7bfe81b82ee9e3001fc07ba 
>   
> ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 
> 3d47991b603c94c8da2106e67192c8513ef783a7 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
> PRE-CREATION 
>   

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/#review202981
---




ql/src/test/queries/clientpositive/erasure_simple.q
Lines 33 (patched)


should be 'select key, value from src;'


- Andrew Sherman


On May 11, 2018, 11:38 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67023/
> ---
> 
> (Updated May 11, 2018, 11:38 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor 
> "ErasureProcessor"
> which allows .q files to contain Erasure Coding commands similar to those 
> provided
> by the hdfs ec command
> https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
> The Erasure Coding functionality is exposed through a new shim 
> "HdfsFileErasureCodingPolicy".
> At this stage there are two .q files:
> erasure_commnds.q (a simple test to show ERASURE commands can run on local fs 
> via
> TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
> erasure_simple.q (which does some trivial queries to demonstrate basic 
> functionality).
> More tests will come in future commits.
> 
> 
> Diffs
> -
> 
>   
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java
>  PRE-CREATION 
>   itests/src/test/resources/testconfiguration.properties 
> cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 
> 6628336807b06cab49063673be0d8e9c5b5a7101 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
> 750fc69c5f210ca8f7bfe81b82ee9e3001fc07ba 
>   
> ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 
> 3d47991b603c94c8da2106e67192c8513ef783a7 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 
> 56c7516ecfaf2421b0f3d3a188d05f38715b25b2 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> 9f65a771f95a7c0bd3fdb4e56e47c0fc70235850 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  de43c2866f64e2ed5c74eab450de28f1a79248dc 
>   ql/src/test/queries/clientpositive/erasure_commands.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/erasure_simple.q PRE-CREATION 
>   ql/src/test/results/clientpositive/erasure_commands.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_commands.q.out 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
> PRE-CREATION 
>   shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
> ec06a88dc21346473bec6589c703167d50e3b367 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
> b89081761780bf1f305d0196bb94bb0b54f7184f 
>   testutils/ptest2/conf/deployed/master-mr2.properties 
> 7edc307f85744d60d322ad8087164625677fc230 
> 
> 
> Diff: https://reviews.apache.org/r/67023/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/
---

(Updated May 11, 2018, 11:38 p.m.)


Review request for hive and Sahil Takiar.


Repository: hive-git


Description
---

TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor 
"ErasureProcessor"
which allows .q files to contain Erasure Coding commands similar to those 
provided
by the hdfs ec command
https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
The Erasure Coding functionality is exposed through a new shim 
"HdfsFileErasureCodingPolicy".
At this stage there are two .q files:
erasure_commnds.q (a simple test to show ERASURE commands can run on local fs 
via
TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
erasure_simple.q (which does some trivial queries to demonstrate basic 
functionality).
More tests will come in future commits.


Diffs (updated)
-

  
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java
 PRE-CREATION 
  itests/src/test/resources/testconfiguration.properties 
cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
  itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 
6628336807b06cab49063673be0d8e9c5b5a7101 
  itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
750fc69c5f210ca8f7bfe81b82ee9e3001fc07ba 
  ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 
3d47991b603c94c8da2106e67192c8513ef783a7 
  ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java 
PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 
56c7516ecfaf2421b0f3d3a188d05f38715b25b2 
  ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
9f65a771f95a7c0bd3fdb4e56e47c0fc70235850 
  
ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
 de43c2866f64e2ed5c74eab450de28f1a79248dc 
  ql/src/test/queries/clientpositive/erasure_commands.q PRE-CREATION 
  ql/src/test/queries/clientpositive/erasure_simple.q PRE-CREATION 
  ql/src/test/results/clientpositive/erasure_commands.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/erasurecoding/erasure_commands.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out 
PRE-CREATION 
  shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 
ec06a88dc21346473bec6589c703167d50e3b367 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java 
b89081761780bf1f305d0196bb94bb0b54f7184f 
  testutils/ptest2/conf/deployed/master-mr2.properties 
7edc307f85744d60d322ad8087164625677fc230 


Diff: https://reviews.apache.org/r/67023/diff/2/

Changes: https://reviews.apache.org/r/67023/diff/1-2/


Testing
---


Thanks,

Andrew Sherman

Re: Review Request 65745: HIVE-18743: CREATE TABLE on S3 data can be extremely slow. DO_NOT_UPDATE_STATS workaround is buggy.

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65745/#review198560
---




standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
Line 657 (original), 652 (patched)


The log message says 'updating table stats fast', as we changed the method 
name perhaps this is now misleading?


- Andrew Sherman


On Feb. 22, 2018, 3:44 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65745/
> ---
> 
> (Updated Feb. 22, 2018, 3:44 a.m.)
> 
> 
> Review request for hive, Andrew Sherman, Janaki Lahorani, Zoltan Haindrich, 
> Sahil Takiar, Thejas Nair, and Vihang Karajgaonkar.
> 
> 
> Bugs: HIVE-18743
> https://issues.apache.org/jira/browse/HIVE-18743
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-18743: CREATE TABLE on S3 data can be extremely slow. 
> DO_NOT_UPDATE_STATS workaround is buggy.
> 
> 
> Diffs
> -
> 
>   
> 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
>  47de215a23afb0d2589f129c75000e36c334888c 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  50f873a013a9aa3cea0a2af8146484b9387c08f2 
> 
> 
> Diff: https://reviews.apache.org/r/65745/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

[Andrew Sherman via Review Board]

> On Jan. 11, 2018, 9:51 p.m., Andrew Sherman wrote:
> > common/src/java/org/apache/hadoop/hive/common/LogUtils.java
> > Lines 259 (patched)
> > 
> >
> > Does deleteAppender() call stop() internally? If so then can we delete 
> > the previous call to stop the subordinateAppender?
> 
> kalyan kumar kalvagadda wrote:
> You are right. deleteAppender internallu calls stop() on the appender. we 
> need to explicitly look-up and stop it. I will update this in my next patch.

Thanks


> On Jan. 11, 2018, 9:51 p.m., Andrew Sherman wrote:
> > itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
> > Line 162 (original), 163 (patched)
> > 
> >
> > This code was to check the case described in 
> > https://issues.apache.org/jira/browse/HIVE-17826?focusedCommentId=16208636=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16208636
> > I think this is no longer tested. Is that OK?
> 
> kalyan kumar kalvagadda wrote:
> I have couple of observations with the limited knowledge that I have. 
> Correct me if i'm wrong here.
> 
> 1. HIVE-17128: Code changes done as part of this jira makes sure that 
> log4j Appender is closed when operation is closed to avoid file descriptors.
> 2. HIVE-17826: Added HushableRandomAccessFileAppender which is a copy of 
> RandomAccessFileAppender but has a explicit check in append() method to see 
> if the appender is closed.
> 
>  I think issue reported in HIVE-17826 is seen only because the 
> RoutingAppender is not properly cleaned-up when the operation is stopped. If 
> we have the patch i submitted we may not see the issue reported in HIVE-17826 
> even with out the fix of adding HushableRandomAccessFileAppender.

OK


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/#review195255
---


On Jan. 11, 2018, 2:11 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65075/
> ---
> 
> (Updated Jan. 11, 2018, 2:11 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.
> 
> 
> Bugs: HIVE-18426
> https://issues.apache.org/jira/browse/HIVE-18426
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
> but when the operation ends, AppenderControl stored in the map is retrieved 
> and stopped but the entry in ConcurrentMap is never cleaned up.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
> 0a3e0c72011951b6b1543352308bd51233c847fb 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
>  8febe3e79ff892c54b696b6c6ef92f7026c46033 
> 
> 
> Diff: https://reviews.apache.org/r/65075/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that the new tests updated to verify this change are passing.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/#review195255
---




common/src/java/org/apache/hadoop/hive/common/LogUtils.java
Lines 259 (patched)


Does deleteAppender() call stop() internally? If so then can we delete the 
previous call to stop the subordinateAppender?



itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
Line 162 (original), 163 (patched)


This code was to check the case described in 
https://issues.apache.org/jira/browse/HIVE-17826?focusedCommentId=16208636=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16208636
I think this is no longer tested. Is that OK?


- Andrew Sherman


On Jan. 11, 2018, 2:11 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65075/
> ---
> 
> (Updated Jan. 11, 2018, 2:11 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.
> 
> 
> Bugs: HIVE-18426
> https://issues.apache.org/jira/browse/HIVE-18426
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
> but when the operation ends, AppenderControl stored in the map is retrieved 
> and stopped but the entry in ConcurrentMap is never cleaned up.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
> 0a3e0c72011951b6b1543352308bd51233c847fb 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
>  8febe3e79ff892c54b696b6c6ef92f7026c46033 
> 
> 
> Diff: https://reviews.apache.org/r/65075/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that the new tests updated to verify this change are passing.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 64193: HIVE-18054: Make Lineage work with concurrent queries on a Session

[Andrew Sherman via Review Board]

> On Dec. 7, 2017, 8:12 p.m., Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMRProcContext.java
> > Line 201 (original), 208 (patched)
> > 
> >
> > Can we use `parseContext.getQueryState()`?

yes, fixed


> On Dec. 7, 2017, 8:12 p.m., Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/spark/GenSparkProcContext.java
> > Lines 164 (patched)
> > 
> >
> > Think u can just use `parseContext.getQueryState()`

yes, fixed


> On Dec. 7, 2017, 8:12 p.m., Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/spark/SparkCompiler.java
> > Lines 351 (patched)
> > 
> >
> > I think `pCtx#getQueryState` could be used for all the changes to the 
> > `TaskCompiler`s?

yes, fixed


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64193/#review193155
---


On Dec. 7, 2017, 7:12 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64193/
> ---
> 
> (Updated Dec. 7, 2017, 7:12 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> A Hive Session can contain multiple concurrent sql Operations.
> Lineage is currently tracked in SessionState and is cleared when a query
> completes. This results in Lineage for other running queries being lost.
> 
> To fix this, move LineageState from SessionState to QueryState.
> In MoveTask/MoveWork use the LineageState from the MoveTask's QueryState
> rather than trying to use it from MoveWork.
> Add a test which runs multiple jdbc queries in a thread pool
> against the same connection and show that Vertices are not lost from Lineage.
> As part of this test, add ReadableHook, an ExecuteWithHookContext that stores
> HookContexts in memory and makes them available for reading.
> Make LineageLogger methods static so they can be used elsewhere.
> 
> Sometimes a running query (originating in a Driver) will instantiate
> another Driver to run or compile another query. Because these Drivers
> shared a Session, the child Driver would accumulate Lineage information
> along with that of the parent Driver. For consistency a LineageState is
> passed to these child Drivers and stored in the new Driver's QueryState.
> 
> 
> Diffs
> -
> 
>   itests/hive-unit/src/test/java/org/apache/hive/jdbc/ReadableHook.java 
> PRE-CREATION 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
> 70bd29c5178456c683652cf2377206059b735514 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> d3df015288fe1963d2b548e32db53cfc2310af21 
>   ql/src/java/org/apache/hadoop/hive/ql/QueryState.java 
> f3a46dbcaf151706521c735654f377a2f2f76a81 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
> 55ef8de9a5c7144931d0a6ff13224765ee737fea 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 
> f5a5e713bb0e081591a53a30caf56f97750c3f8e 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Task.java 
> 1f0487f4f72ab18bcf876f45ad5758d83a7f001b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java
>  262225fc202d4627652acfd77350e44b0284b3da 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadTable.java
>  545b7a8b7e9f1370b767fc777cb10fa59bd81917 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/HookContext.java 
> 7b617309f6b0d8a7ce0dea80ab1f790c2651b147 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/LineageLogger.java 
> 2f764f8a29a9d41a7db013a949ffe3a8a9417d32 
>   ql/src/java/org/apache/hadoop/hive/ql/index/AggregateIndexHandler.java 
> 68709b4d3baf15d78e60e948ccdef3df84f28cec 
>   ql/src/java/org/apache/hadoop/hive/ql/index/HiveIndexHandler.java 
> 1e577da82343a1b7361467fb662661f9c6642ec0 
>   ql/src/java/org/apache/hadoop/hive/ql/index/TableBasedIndexHandler.java 
> 29886ae7f97f8dae7116f4fc9a2417ab8f9dac0a 
>   ql/src/java/org/apache/hadoop/hive/ql/index/bitmap/BitmapIndexHandler.java 
> 7b067a0d45e33bc3347c43b050af933c296a9227 
>   
> ql/src/java/org/apache/hadoop/hive/ql/index/compact/CompactIndexHandler.java 
> 504b0623142a6fa6cdb45a26b49f146e12ec2d7a 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMRFileSink1.java 
> d7a83f775abca39b219f71aff88173a14ffaee9f 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMRProcContext.java 
> 4387c4297fee48d4c03e95d5a2fcb822ab480eeb 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java 
> bdaf105697fd2c2074885fa3a35548043167c7e7 
>   

Re: Review Request 64193: HIVE-18054: Make Lineage work with concurrent queries on a Session

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64193/
---

(Updated Dec. 7, 2017, 7:12 p.m.)


Review request for hive.


Repository: hive-git


Description
---

A Hive Session can contain multiple concurrent sql Operations.
Lineage is currently tracked in SessionState and is cleared when a query
completes. This results in Lineage for other running queries being lost.

To fix this, move LineageState from SessionState to QueryState.
In MoveTask/MoveWork use the LineageState from the MoveTask's QueryState
rather than trying to use it from MoveWork.
Add a test which runs multiple jdbc queries in a thread pool
against the same connection and show that Vertices are not lost from Lineage.
As part of this test, add ReadableHook, an ExecuteWithHookContext that stores
HookContexts in memory and makes them available for reading.
Make LineageLogger methods static so they can be used elsewhere.

Sometimes a running query (originating in a Driver) will instantiate
another Driver to run or compile another query. Because these Drivers
shared a Session, the child Driver would accumulate Lineage information
along with that of the parent Driver. For consistency a LineageState is
passed to these child Drivers and stored in the new Driver's QueryState.


Diffs (updated)
-

  itests/hive-unit/src/test/java/org/apache/hive/jdbc/ReadableHook.java 
PRE-CREATION 
  itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
70bd29c5178456c683652cf2377206059b735514 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
d3df015288fe1963d2b548e32db53cfc2310af21 
  ql/src/java/org/apache/hadoop/hive/ql/QueryState.java 
f3a46dbcaf151706521c735654f377a2f2f76a81 
  ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
55ef8de9a5c7144931d0a6ff13224765ee737fea 
  ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 
f5a5e713bb0e081591a53a30caf56f97750c3f8e 
  ql/src/java/org/apache/hadoop/hive/ql/exec/Task.java 
1f0487f4f72ab18bcf876f45ad5758d83a7f001b 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java
 262225fc202d4627652acfd77350e44b0284b3da 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadTable.java
 545b7a8b7e9f1370b767fc777cb10fa59bd81917 
  ql/src/java/org/apache/hadoop/hive/ql/hooks/HookContext.java 
7b617309f6b0d8a7ce0dea80ab1f790c2651b147 
  ql/src/java/org/apache/hadoop/hive/ql/hooks/LineageLogger.java 
2f764f8a29a9d41a7db013a949ffe3a8a9417d32 
  ql/src/java/org/apache/hadoop/hive/ql/index/AggregateIndexHandler.java 
68709b4d3baf15d78e60e948ccdef3df84f28cec 
  ql/src/java/org/apache/hadoop/hive/ql/index/HiveIndexHandler.java 
1e577da82343a1b7361467fb662661f9c6642ec0 
  ql/src/java/org/apache/hadoop/hive/ql/index/TableBasedIndexHandler.java 
29886ae7f97f8dae7116f4fc9a2417ab8f9dac0a 
  ql/src/java/org/apache/hadoop/hive/ql/index/bitmap/BitmapIndexHandler.java 
7b067a0d45e33bc3347c43b050af933c296a9227 
  ql/src/java/org/apache/hadoop/hive/ql/index/compact/CompactIndexHandler.java 
504b0623142a6fa6cdb45a26b49f146e12ec2d7a 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMRFileSink1.java 
d7a83f775abca39b219f71aff88173a14ffaee9f 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMRProcContext.java 
4387c4297fee48d4c03e95d5a2fcb822ab480eeb 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java 
bdaf105697fd2c2074885fa3a35548043167c7e7 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/IndexUtils.java 
338c1856672f09bb7da35d2336ebb5b6f3fdc5a6 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java 
e6c07713b24df719315d804f006151106eea9aed 
  ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 
a09b7961c2dbc26b4d2fa912d0be7037885f63e4 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java 
065c7e50986872cd35386feee712f3452597d643 
  ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezProcContext.java 
0c160acf46eb1eb07c5f04091099c1024e166638 
  ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 
b6f1139fe1a78283277bf4d0c5224ab1d718c634 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 
83d53bc157f35b4b57fc37bb24b6c400ac58d8ca 
  ql/src/java/org/apache/hadoop/hive/ql/parse/IndexUpdater.java 
f31775ed942160da73344c4dca707da7b8c658a6 
  ql/src/java/org/apache/hadoop/hive/ql/parse/LoadSemanticAnalyzer.java 
cc956da57567114aa29ee0552566ca62c68f6be7 
  ql/src/java/org/apache/hadoop/hive/ql/parse/MapReduceCompiler.java 
d7a56e5846d5754dec5070d8c3543a3695e4 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ReplicationSemanticAnalyzer.java 
498b6741c3f40b92ce3fb218e91e7809a17383f0 
  ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
28e3621d3264f4f704da0d775b396f7b7764fdb6 
  ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 

Re: Review Request 64193: HIVE-18054: Make Lineage work with concurrent queries on a Session

[Andrew Sherman via Review Board]

> On Dec. 2, 2017, 12:22 a.m., Sahil Takiar wrote:
> > Since we touch the `LoadSemanticAnalyzer` could we add a q-test (could be 
> > added to one of the existing `lineage*.q` files) for `LOAD` statements. 
> > Same for import / export statements (as far as I can tell there are no 
> > existing ones, correct me if I am wrong).
> > 
> > If you have time, it would be great to run some of the lineage tests for 
> > HoS too, but since thats a bit orthogonal to this JIRA, it can be done in a 
> > follow up JIRA.
> 
> Andrew Sherman wrote:
> I will addsome more tests...

Update: I looked into his some more. It turns out that Lineage info is printed 
by default by the PostExecutePrinter.
So any .q tests should show their lineage in the output.
HoS tests do print some lineage so I think that part is covered.
But currently no lineage is printed for LOAD/IMPORT/EXPORT.
The MoveTask does call getLineageState().setLineage() to set lineage into the 
LineageState but it does not call getLineageState().mapDirToOp().
Possibly this is because LOAD statements don't have a 
org.apache.hadoop.hive.ql.exec.Operator
And IMPORT statements behave similarly.
IMO the whole Lineage stuff is not clearly specified. 
Do you think it is worth doing follow-up work to try to document /test) it 
better?


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64193/#review192601
---


On Nov. 30, 2017, 1:22 a.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64193/
> ---
> 
> (Updated Nov. 30, 2017, 1:22 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> A Hive Session can contain multiple concurrent sql Operations.
> Lineage is currently tracked in SessionState and is cleared when a query
> completes. This results in Lineage for other running queries being lost.
> 
> To fix this, move LineageState from SessionState to QueryState.
> In MoveTask/MoveWork use the LineageState from the MoveTask's QueryState
> rather than trying to use it from MoveWork.
> Add a test which runs multiple jdbc queries in a thread pool
> against the same connection and show that Vertices are not lost from Lineage.
> As part of this test, add ReadableHook, an ExecuteWithHookContext that stores
> HookContexts in memory and makes them available for reading.
> Make LineageLogger methods static so they can be used elsewhere.
> 
> Sometimes a running query (originating in a Driver) will instantiate
> another Driver to run or compile another query. Because these Drivers
> shared a Session, the child Driver would accumulate Lineage information
> along with that of the parent Driver. For consistency a LineageState is
> passed to these child Drivers and stored in the new Driver's QueryState.
> 
> 
> Diffs
> -
> 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
> f5ed735c1ec14dfee338e56020fa2629b168389d 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> af9f193dc94e2e05caa88d965a34f4483c9d7069 
>   ql/src/java/org/apache/hadoop/hive/ql/QueryState.java 
> 7d5aa8b179e536e25c41a8946e667f8dd5669e0f 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
> e7af5e004fb560b574b82f6d1b60517511802f37 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 
> e2f8c1f8012ad25114e279747e821b291c7f4ca6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Task.java 
> 1f0487f4f72ab18bcf876f45ad5758d83a7f001b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java
>  262225fc202d4627652acfd77350e44b0284b3da 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadTable.java
>  bb1f4e50509e57a9d0b9e6793c1fc08baa4d2981 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/HookContext.java 
> 7b617309f6b0d8a7ce0dea80ab1f790c2651b147 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/LineageLogger.java 
> 2f764f8a29a9d41a7db013a949ffe3a8a9417d32 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadableHook.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/index/AggregateIndexHandler.java 
> 68709b4d3baf15d78e60e948ccdef3df84f28cec 
>   ql/src/java/org/apache/hadoop/hive/ql/index/HiveIndexHandler.java 
> 1e577da82343a1b7361467fb662661f9c6642ec0 
>   ql/src/java/org/apache/hadoop/hive/ql/index/TableBasedIndexHandler.java 
> 29886ae7f97f8dae7116f4fc9a2417ab8f9dac0a 
>   ql/src/java/org/apache/hadoop/hive/ql/index/bitmap/BitmapIndexHandler.java 
> 7b067a0d45e33bc3347c43b050af933c296a9227 
>   
> ql/src/java/org/apache/hadoop/hive/ql/index/compact/CompactIndexHandler.java 
> 504b0623142a6fa6cdb45a26b49f146e12ec2d7a 
>   

Re: Review Request 64193: HIVE-18054: Make Lineage work with concurrent queries on a Session

[Andrew Sherman via Review Board]

> On Dec. 2, 2017, 12:22 a.m., Sahil Takiar wrote:
> > Since we touch the `LoadSemanticAnalyzer` could we add a q-test (could be 
> > added to one of the existing `lineage*.q` files) for `LOAD` statements. 
> > Same for import / export statements (as far as I can tell there are no 
> > existing ones, correct me if I am wrong).
> > 
> > If you have time, it would be great to run some of the lineage tests for 
> > HoS too, but since thats a bit orthogonal to this JIRA, it can be done in a 
> > follow up JIRA.

I will addsome more tests...


> On Dec. 2, 2017, 12:22 a.m., Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 365 (patched)
> > 
> >
> > Sounds good. Just curious, is there any way to know for sure where code 
> > run by a `Driver`, creates another `Driver`? How did you determine when 
> > this is necessary?

I reviewed all code that creates a Driver.


> On Dec. 2, 2017, 12:22 a.m., Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java
> > Line 401 (original), 401 (patched)
> > 
> >
> > Ok, but do we need to do `if (queryState.getLineageState() != null)` to 
> > ensure an NPE isn't thrown? That seems to be what the old code is doing.

I don't think we need to do that. There is always an initial lineageState 
inside queryState so for it to be null someone would have had to call 
setLineageState(null)


> On Dec. 2, 2017, 12:22 a.m., Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java
> > Lines 111 (patched)
> > 
> >
> > Doesn't a `TaskCompiler` already have a `QueryState` object? Why do we 
> > need to explicitly pass in a `LineageState`?

Good catch, I will fix


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64193/#review192601
---


On Nov. 30, 2017, 1:22 a.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64193/
> ---
> 
> (Updated Nov. 30, 2017, 1:22 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> A Hive Session can contain multiple concurrent sql Operations.
> Lineage is currently tracked in SessionState and is cleared when a query
> completes. This results in Lineage for other running queries being lost.
> 
> To fix this, move LineageState from SessionState to QueryState.
> In MoveTask/MoveWork use the LineageState from the MoveTask's QueryState
> rather than trying to use it from MoveWork.
> Add a test which runs multiple jdbc queries in a thread pool
> against the same connection and show that Vertices are not lost from Lineage.
> As part of this test, add ReadableHook, an ExecuteWithHookContext that stores
> HookContexts in memory and makes them available for reading.
> Make LineageLogger methods static so they can be used elsewhere.
> 
> Sometimes a running query (originating in a Driver) will instantiate
> another Driver to run or compile another query. Because these Drivers
> shared a Session, the child Driver would accumulate Lineage information
> along with that of the parent Driver. For consistency a LineageState is
> passed to these child Drivers and stored in the new Driver's QueryState.
> 
> 
> Diffs
> -
> 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
> f5ed735c1ec14dfee338e56020fa2629b168389d 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> af9f193dc94e2e05caa88d965a34f4483c9d7069 
>   ql/src/java/org/apache/hadoop/hive/ql/QueryState.java 
> 7d5aa8b179e536e25c41a8946e667f8dd5669e0f 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
> e7af5e004fb560b574b82f6d1b60517511802f37 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 
> e2f8c1f8012ad25114e279747e821b291c7f4ca6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Task.java 
> 1f0487f4f72ab18bcf876f45ad5758d83a7f001b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java
>  262225fc202d4627652acfd77350e44b0284b3da 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadTable.java
>  bb1f4e50509e57a9d0b9e6793c1fc08baa4d2981 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/HookContext.java 
> 7b617309f6b0d8a7ce0dea80ab1f790c2651b147 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/LineageLogger.java 
> 2f764f8a29a9d41a7db013a949ffe3a8a9417d32 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadableHook.java PRE-CREATION 
>   

Re: Review Request 64193: HIVE-18054: Make Lineage work with concurrent queries on a Session

[Andrew Sherman via Review Board]

> On Dec. 1, 2017, 5:37 p.m., Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 365 (patched)
> > 
> >
> > Why do we need constructors to explicitly pass in a `LineageState` 
> > object. I thought that a `LineageState` is part of a `QueryState` oject 
> > now. Is this what you are referring to as passing the `LineageState` from a 
> > parent `Driver` to a child `Driver`?

Yes, we need to pass lineageState when a driver instantiates another Driver to 
run or compile another query. I've added a comment to the new constructors to 
explain this.


> On Dec. 1, 2017, 5:37 p.m., Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java
> > Line 401 (original), 401 (patched)
> > 
> >
> > Why is this necessary?

You are asking about work.getLineagState() != null ? This was some 
comparatively recent code which put the LineageState in the *Work classes.


> On Dec. 1, 2017, 5:37 p.m., Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadableHook.java
> > Lines 27 (patched)
> > 
> >
> > If this is just used in tests, why not make it a test-specific class.

At one point I needed to have the hook in the HS2 classpath, that is no longer 
true so I moved it to test, thanks.
.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64193/#review192514
---


On Nov. 30, 2017, 1:22 a.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64193/
> ---
> 
> (Updated Nov. 30, 2017, 1:22 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> A Hive Session can contain multiple concurrent sql Operations.
> Lineage is currently tracked in SessionState and is cleared when a query
> completes. This results in Lineage for other running queries being lost.
> 
> To fix this, move LineageState from SessionState to QueryState.
> In MoveTask/MoveWork use the LineageState from the MoveTask's QueryState
> rather than trying to use it from MoveWork.
> Add a test which runs multiple jdbc queries in a thread pool
> against the same connection and show that Vertices are not lost from Lineage.
> As part of this test, add ReadableHook, an ExecuteWithHookContext that stores
> HookContexts in memory and makes them available for reading.
> Make LineageLogger methods static so they can be used elsewhere.
> 
> Sometimes a running query (originating in a Driver) will instantiate
> another Driver to run or compile another query. Because these Drivers
> shared a Session, the child Driver would accumulate Lineage information
> along with that of the parent Driver. For consistency a LineageState is
> passed to these child Drivers and stored in the new Driver's QueryState.
> 
> 
> Diffs
> -
> 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
> f5ed735c1ec14dfee338e56020fa2629b168389d 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> af9f193dc94e2e05caa88d965a34f4483c9d7069 
>   ql/src/java/org/apache/hadoop/hive/ql/QueryState.java 
> 7d5aa8b179e536e25c41a8946e667f8dd5669e0f 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
> e7af5e004fb560b574b82f6d1b60517511802f37 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 
> e2f8c1f8012ad25114e279747e821b291c7f4ca6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Task.java 
> 1f0487f4f72ab18bcf876f45ad5758d83a7f001b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java
>  262225fc202d4627652acfd77350e44b0284b3da 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadTable.java
>  bb1f4e50509e57a9d0b9e6793c1fc08baa4d2981 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/HookContext.java 
> 7b617309f6b0d8a7ce0dea80ab1f790c2651b147 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/LineageLogger.java 
> 2f764f8a29a9d41a7db013a949ffe3a8a9417d32 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadableHook.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/index/AggregateIndexHandler.java 
> 68709b4d3baf15d78e60e948ccdef3df84f28cec 
>   ql/src/java/org/apache/hadoop/hive/ql/index/HiveIndexHandler.java 
> 1e577da82343a1b7361467fb662661f9c6642ec0 
>   ql/src/java/org/apache/hadoop/hive/ql/index/TableBasedIndexHandler.java 
> 29886ae7f97f8dae7116f4fc9a2417ab8f9dac0a 
>   ql/src/java/org/apache/hadoop/hive/ql/index/bitmap/BitmapIndexHandler.java 
> 

Review Request 64193: HIVE-18054: Make Lineage work with concurrent queries on a Session

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64193/
---

Review request for hive.


Repository: hive-git


Description
---

A Hive Session can contain multiple concurrent sql Operations.
Lineage is currently tracked in SessionState and is cleared when a query
completes. This results in Lineage for other running queries being lost.

To fix this, move LineageState from SessionState to QueryState.
In MoveTask/MoveWork use the LineageState from the MoveTask's QueryState
rather than trying to use it from MoveWork.
Add a test which runs multiple jdbc queries in a thread pool
against the same connection and show that Vertices are not lost from Lineage.
As part of this test, add ReadableHook, an ExecuteWithHookContext that stores
HookContexts in memory and makes them available for reading.
Make LineageLogger methods static so they can be used elsewhere.

Sometimes a running query (originating in a Driver) will instantiate
another Driver to run or compile another query. Because these Drivers
shared a Session, the child Driver would accumulate Lineage information
along with that of the parent Driver. For consistency a LineageState is
passed to these child Drivers and stored in the new Driver's QueryState.


Diffs
-

  itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
f5ed735c1ec14dfee338e56020fa2629b168389d 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
af9f193dc94e2e05caa88d965a34f4483c9d7069 
  ql/src/java/org/apache/hadoop/hive/ql/QueryState.java 
7d5aa8b179e536e25c41a8946e667f8dd5669e0f 
  ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 
e7af5e004fb560b574b82f6d1b60517511802f37 
  ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 
e2f8c1f8012ad25114e279747e821b291c7f4ca6 
  ql/src/java/org/apache/hadoop/hive/ql/exec/Task.java 
1f0487f4f72ab18bcf876f45ad5758d83a7f001b 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java
 262225fc202d4627652acfd77350e44b0284b3da 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadTable.java
 bb1f4e50509e57a9d0b9e6793c1fc08baa4d2981 
  ql/src/java/org/apache/hadoop/hive/ql/hooks/HookContext.java 
7b617309f6b0d8a7ce0dea80ab1f790c2651b147 
  ql/src/java/org/apache/hadoop/hive/ql/hooks/LineageLogger.java 
2f764f8a29a9d41a7db013a949ffe3a8a9417d32 
  ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadableHook.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/index/AggregateIndexHandler.java 
68709b4d3baf15d78e60e948ccdef3df84f28cec 
  ql/src/java/org/apache/hadoop/hive/ql/index/HiveIndexHandler.java 
1e577da82343a1b7361467fb662661f9c6642ec0 
  ql/src/java/org/apache/hadoop/hive/ql/index/TableBasedIndexHandler.java 
29886ae7f97f8dae7116f4fc9a2417ab8f9dac0a 
  ql/src/java/org/apache/hadoop/hive/ql/index/bitmap/BitmapIndexHandler.java 
7b067a0d45e33bc3347c43b050af933c296a9227 
  ql/src/java/org/apache/hadoop/hive/ql/index/compact/CompactIndexHandler.java 
504b0623142a6fa6cdb45a26b49f146e12ec2d7a 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMRFileSink1.java 
d7a83f775abca39b219f71aff88173a14ffaee9f 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMRProcContext.java 
4387c4297fee48d4c03e95d5a2fcb822ab480eeb 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java 
67739a1db9fc52a67f4f5ea7dba80fe0e95750c8 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/IndexUtils.java 
338c1856672f09bb7da35d2336ebb5b6f3fdc5a6 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java 
e6c07713b24df719315d804f006151106eea9aed 
  ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 
1fd634c928a5384b09d97322c3ea785f518d73fe 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java 
065c7e50986872cd35386feee712f3452597d643 
  ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezProcContext.java 
0c160acf46eb1eb07c5f04091099c1024e166638 
  ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 
b6f1139fe1a78283277bf4d0c5224ab1d718c634 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 
cd75130d7c5f0b402f1b4331c57edc611eb4b2ed 
  ql/src/java/org/apache/hadoop/hive/ql/parse/IndexUpdater.java 
f31775ed942160da73344c4dca707da7b8c658a6 
  ql/src/java/org/apache/hadoop/hive/ql/parse/LoadSemanticAnalyzer.java 
238fbd60572ee5f7f8f6c4d5b2abce8f66c7e495 
  ql/src/java/org/apache/hadoop/hive/ql/parse/MapReduceCompiler.java 
d7a56e5846d5754dec5070d8c3543a3695e4 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ReplicationSemanticAnalyzer.java 
498b6741c3f40b92ce3fb218e91e7809a17383f0 
  ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
b66817f65f65b6aaf8dbc339a969b8e9e0565e9e 
  ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 
7b2937032ab8dd57f8923e0a9e7aab4a92de55ee 
  

Re: Review Request 63586: Fix HIVE-17942: HiveAlterHandler should use the conf from HMS Handler

[Andrew Sherman via Review Board]

---
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

[Andrew Sherman via Review Board]

---
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)


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
> 
>

Re: Review Request 62995: HIVE-17806 Create directory for metrics file if it doesn't exist

[Andrew Sherman via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/#review188223
---




common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 89 (original), 92 (patched)


comment refers to old name



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 102 (original), 105 (patched)


should this say "in" the same directory ?



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 173 (original), 188 (patched)


should be "temporary" not yemporary



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
Line 164 (original), 179 (patched)


should be"temporary"


- Andrew Sherman


On Oct. 13, 2017, 11:18 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62995/
> ---
> 
> (Updated Oct. 13, 2017, 11:18 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio 
> Pena, Sahil Takiar, and Vihang Karajgaonkar.
> 
> 
> Bugs: HIVE-17806
> https://issues.apache.org/jira/browse/HIVE-17806
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-17806 Create directory for metrics file if it doesn't exist
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
>  96243cb74a154b9a639ffb080256c4b43bd35a4b 
>   
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
>  254af7d4310578e3883c0dffa64bed0f823696ea 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
>  f71bb25463b56bc741f989454664397996b6a5cf 
> 
> 
> Diff: https://reviews.apache.org/r/62995/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62693: HIVE-17635: Add unit tests to CompactionTxnHandler and use PreparedStatements for queries

[Andrew Sherman via Review Board]

> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 387 (original), 411 (patched)
> > 
> >
> > What is this for?

The 'questions' list is used to create the list containing 'IN list values'. If 
not using PreparedStatements these would be actual values. In the 
PreparedStatement case they are a list of ?. This is arguably ugly but it 
allows us to use common code for PreparedStatements and (unprepared) 
Statements. See below for a more complete explanantion.


> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 413 (original), 444 (patched)
> > 
> >
> > Is this necessary?

No, its a mistake, thanks


> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 432 (original), 475 (patched)
> > 
> >
> > If we are changing this, should we just use try-with-resources.

I agree try-with-resources is great, I wanted to mimimize my changes


> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
> > Lines 158 (patched)
> > 
> >
> > why is a new return value necessary?

We are looking at code that generates IN clauses: 
  select count(*) from TXNS where (TXN_ID in (1,2,3,4,5)) and TXN_STATE = 'o'
There are limits on how many values you can have in an IN clause (like maybe 
1000), and the code knows something about that.
If you ask it to generate code for a lot of values then it returns multiple 
queries:
  select count(*) from TXNS where (TXN_ID in (1,2,3,4,5)) and TXN_STATE = 'o'
  select count(*) from TXNS where (TXN_ID in (1001,1002,1003) and TXN_STATE = 
'o'
My change involves using the same logic to build PreparedStatements. These look 
like:
  select count(*) from TXNS where (TXN_ID in (?,?,?,?,?)) and TXN_STATE = 'o'
  select count(*) from TXNS where (TXN_ID in (?,?,?)) and TXN_STATE = 'o'
The difference is that with PreparedStatements the code must also subsequently 
call 
 pStmt.setLong(paramNum, value)
The right number of times for each query. So the new method 
buildQueryWithINClauseStrings,  
in addition to building the list of queries also returns a corresponding list 
of the number of ? i
n the the generated in clause.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62693/#review187818
---


On Sept. 29, 2017, 4:51 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62693/
> ---
> 
> (Updated Sept. 29, 2017, 4:51 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Add a unit test which exercises CompactionTxnHandler.markFailed() and change 
> it to use PreparedStament.
> Add test for checkFailedCompactions() and change it to use PreparedStatement
> Add a unit test which exercises purgeCompactionHistory().
> Add buildQueryWithINClauseStrings() which is suitable for building in clauses 
> for PreparedStatement
> Add test code to TestTxnUtils to tickle code in 
> TxnUtils.buildQueryWithINClauseStrings() so that it produces multiple queries.
> Change markCleaned() to use PreparedStatement
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/HiveSchemaTool.java 
> 84963af10ec13979a7b3976be434efbc21cf2382 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  60839faa352cbf959041a455e9e780dfca0afdc3 
>   metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java 
> 30b155f3b3311fed6cd79e46a5b2abcee9927d91 
>   metastore/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnUtils.java 
> 1497c00e5dc77c02e53767b014a23e5fd8cb5b29 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  f8ae86bea3fe78374c0e0487d66c661f4f0d78ff 
> 
> 
> Diff: https://reviews.apache.org/r/62693/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>