from:"Adam Szita via Review Board"

Re: Review Request 72276: HIVE-23084: Implement kill query in multiple HS2 environment

2020-04-14 Thread Adam Szita via Review Board



> On April 9, 2020, 9:09 a.m., Adam Szita wrote:
> > Ship It!

Committed. This can be closed.


- Adam


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


On April 6, 2020, 10:04 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72276/
> ---
> 
> (Updated April 6, 2020, 10:04 a.m.)
> 
> 
> Review request for hive and Adam Szita.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> KILL  command was implemented in:
> 
> https://issues.apache.org/jira/browse/HIVE-17483
> https://issues.apache.org/jira/browse/HIVE-20549
> But it is not working in an environment where service discovery is enabled 
> and more than one HS2 instance is running (except for manually sending the 
> kill query to all HS2 instance).
> 
> Solution:
> 
> If a HS2 instance can't kill a query locally, it should post a kill query 
> request to the Zookeeper
> Every HS2 should watch the Zookeeper for kill query requests and if its 
> running on that instance kill it
> Authorization of kill query should work the same
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 73f185a1f3 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/BaseJdbcWithMiniLlap.java 
> 3973ec9270 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlapArrow.java
>  68a515ccbe 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithServiceDiscovery.java
>  PRE-CREATION 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/TestMiniHS2StateWithNoZookeeper.java
>  99e681e5b2 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/server/TestKillQueryZookeeperManager.java
>  PRE-CREATION 
>   itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
> 1b60a51ebd 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java db965e7a22 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/process/kill/KillQueriesOperation.java
>  afde1a4762 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 8becef1cd3 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 9e497545b5 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
> 277519cba5 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 181ea5d6d5 
>   service/src/java/org/apache/hive/service/server/KillQueryImpl.java 
> 883e32bd2e 
>   
> service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/ZooKeeperHiveHelper.java
>  71d8651712 
> 
> 
> Diff: https://reviews.apache.org/r/72276/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72276: HIVE-23084: Implement kill query in multiple HS2 environment

2020-04-09 Thread Adam Szita via Review Board


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


Ship it!




Ship It!

- Adam Szita


On April 6, 2020, 10:04 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72276/
> ---
> 
> (Updated April 6, 2020, 10:04 a.m.)
> 
> 
> Review request for hive and Adam Szita.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> KILL  command was implemented in:
> 
> https://issues.apache.org/jira/browse/HIVE-17483
> https://issues.apache.org/jira/browse/HIVE-20549
> But it is not working in an environment where service discovery is enabled 
> and more than one HS2 instance is running (except for manually sending the 
> kill query to all HS2 instance).
> 
> Solution:
> 
> If a HS2 instance can't kill a query locally, it should post a kill query 
> request to the Zookeeper
> Every HS2 should watch the Zookeeper for kill query requests and if its 
> running on that instance kill it
> Authorization of kill query should work the same
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 73f185a1f3 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/BaseJdbcWithMiniLlap.java 
> 3973ec9270 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlapArrow.java
>  68a515ccbe 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithServiceDiscovery.java
>  PRE-CREATION 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/TestMiniHS2StateWithNoZookeeper.java
>  99e681e5b2 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/server/TestKillQueryZookeeperManager.java
>  PRE-CREATION 
>   itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
> 1b60a51ebd 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java db965e7a22 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/process/kill/KillQueriesOperation.java
>  afde1a4762 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 8becef1cd3 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 9e497545b5 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
> 277519cba5 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 181ea5d6d5 
>   service/src/java/org/apache/hive/service/server/KillQueryImpl.java 
> 883e32bd2e 
>   
> service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/ZooKeeperHiveHelper.java
>  71d8651712 
> 
> 
> Diff: https://reviews.apache.org/r/72276/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72276: HIVE-23084: Implement kill query in multiple HS2 environment

2020-03-30 Thread Adam Szita via Review Board


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



Looking pretty good overall, I just have a few questions/comments.


itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlapArrow.java
Lines 415 (patched)


Might be worth to extract the expected string as a constant?



itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithServiceDiscovery.java
Lines 94 (patched)


Can be private if not used elsewhere



service/src/java/org/apache/hive/service/server/KillQueryImpl.java
Line 150 (original), 176 (patched)


Shouldn't we return if there are no ops to kill? I think a subsequent 
killOperations() call here might throw an NPE.



service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
Lines 106 (patched)


nit: typo: namespace



service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
Lines 243 (patched)


I'm fine with not exposing these in HiveConf (that's already a monster) but 
we could at least extract these as constants in this class.



service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
Lines 449 (patched)


Shouldn't we clear the progress flag here?



service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
Lines 453 (patched)


this is a no-op here


- Adam Szita


On March 27, 2020, 10:08 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72276/
> ---
> 
> (Updated March 27, 2020, 10:08 a.m.)
> 
> 
> Review request for hive and Adam Szita.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> KILL  command was implemented in:
> 
> https://issues.apache.org/jira/browse/HIVE-17483
> https://issues.apache.org/jira/browse/HIVE-20549
> But it is not working in an environment where service discovery is enabled 
> and more than one HS2 instance is running (except for manually sending the 
> kill query to all HS2 instance).
> 
> Solution:
> 
> If a HS2 instance can't kill a query locally, it should post a kill query 
> request to the Zookeeper
> Every HS2 should watch the Zookeeper for kill query requests and if its 
> running on that instance kill it
> Authorization of kill query should work the same
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 34df01e60e 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/BaseJdbcWithMiniLlap.java 
> 3973ec9270 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlapArrow.java
>  68a515ccbe 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithServiceDiscovery.java
>  PRE-CREATION 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/TestMiniHS2StateWithNoZookeeper.java
>  99e681e5b2 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/server/TestKillQueryZookeeperManager.java
>  PRE-CREATION 
>   itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
> 1b60a51ebd 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 8becef1cd3 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 9e497545b5 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
> 277519cba5 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 181ea5d6d5 
>   service/src/java/org/apache/hive/service/server/KillQueryImpl.java 
> 883e32bd2e 
>   
> service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/ZooKeeperHiveHelper.java
>  71d8651712 
> 
> 
> Diff: https://reviews.apache.org/r/72276/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 71575: HIVE-22284: Improve LLAP CacheContentsTracker to collect and display correct statistics

2019-10-08 Thread Adam Szita via Review Board


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

(Updated Oct. 8, 2019, 7:53 a.m.)


Review request for hive.


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


Repository: hive-git


Description
---

When keeping track of which buffers correspond to what Hive objects, 
CacheContentsTracker relies on cache tags.

Currently a tag is a simple String that ideally holds DB and table name, and a 
partition spec concatenated by . and / . The information here is derived from 
the Path of the file that is getting cached. Needless to say sometimes this 
produces a wrong tag especially for external tables.

Also there's a bug when calculating aggregated stats for a 'parent' tag 
(corresponding to the table of the partition) because the overall maxCount and 
maxSize do not add up to the sum of those in the partitions. This happens when 
buffers get removed from the cache.


Diffs (updated)
-

  llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java 
a351a193c6bc558bb420049c54b7657cd7d04b7c 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/CacheContentsTracker.java
 64c0125833af100fd7012b9751d075ab536ad1b0 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapCacheableBuffer.java 
f91a5d91a5b739dcbee98a1485ad4c59f6a9057b 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapDataBuffer.java 
405fca2d4fae9fe0e3fd6d6d1345d55255d6df78 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCache.java 
4dd3826a67dfff66ce9c90027d61a9012c0a15e8 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCacheImpl.java 
62d7e5534486b53634de332875c5fd5d336c29b4 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/SerDeLowLevelCacheImpl.java
 2a39d2d32807a51346baad28b04d87670381b6d5 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/SimpleBufferManager.java 
41855e171eaa5bf8da638bc62bce3d0d49dc4bae 
  llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
c63ee5f79b4f9fc356f033960e0af1a7b0058038 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapRecordReader.java
 1378a01f44ef774a15f769460833064c6305b2d6 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/OrcColumnVectorProducer.java
 2a0c5ca92f3c7431f3c399f309a538f47eb27597 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/encoded/OrcEncodedDataReader.java
 85a42f945624c3ca468790772f52363b4064d8fc 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/encoded/SerDeEncodedDataReader.java
 d414b1405b7672767196b3eaad02baa516169288 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/MetadataCache.java 
8400fe98411ed07bd525a51a223fc35423136efb 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/OrcFileEstimateErrors.java
 30dc1b9da2002689b8b1917f46ae3ca24194f3be 
  
llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestCacheContentsTracker.java
 PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/llap/LlapCacheAwareFs.java 
af04a51b5536550b2d2f7d3e008cf2b2dea607d4 
  ql/src/java/org/apache/hadoop/hive/llap/LlapHiveUtils.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/EncodedReaderImpl.java 
241a3001e6e0002377736d6d0e820fde004b0bac 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/Reader.java 
210c987b7f580dacda5bdb487af9cf234a738b79 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/ReaderImpl.java 
a9a9f101948a970e0dbf2f77eeb6f688a88d1cbd 
  
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/VectorizedParquetRecordReader.java
 61e2556b08fe4247f35673f24378505ada20a605 
  storage-api/src/java/org/apache/hadoop/hive/common/io/CacheTag.java 
PRE-CREATION 
  storage-api/src/java/org/apache/hadoop/hive/common/io/DataCache.java 
2ac0a18a5026e76e65c3c3a8b81d5a844c472ed2 
  storage-api/src/java/org/apache/hadoop/hive/common/io/FileMetadataCache.java 
d7de3619380d24a1aeea2bac9a66485d7d468517 


Diff: https://reviews.apache.org/r/71575/diff/4/

Changes: https://reviews.apache.org/r/71575/diff/3-4/


Testing
---


Thanks,

Adam Szita

Re: Review Request 71575: HIVE-22284: Improve LLAP CacheContentsTracker to collect and display correct statistics

2019-10-07 Thread Adam Szita via Review Board


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

(Updated Oct. 7, 2019, 2:26 p.m.)


Review request for hive.


Changes
---

Reworked CacheTag to minimize memory overhead footprint


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


Repository: hive-git


Description
---

When keeping track of which buffers correspond to what Hive objects, 
CacheContentsTracker relies on cache tags.

Currently a tag is a simple String that ideally holds DB and table name, and a 
partition spec concatenated by . and / . The information here is derived from 
the Path of the file that is getting cached. Needless to say sometimes this 
produces a wrong tag especially for external tables.

Also there's a bug when calculating aggregated stats for a 'parent' tag 
(corresponding to the table of the partition) because the overall maxCount and 
maxSize do not add up to the sum of those in the partitions. This happens when 
buffers get removed from the cache.


Diffs (updated)
-

  llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java 
a351a193c6bc558bb420049c54b7657cd7d04b7c 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/CacheContentsTracker.java
 64c0125833af100fd7012b9751d075ab536ad1b0 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapCacheableBuffer.java 
f91a5d91a5b739dcbee98a1485ad4c59f6a9057b 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapDataBuffer.java 
405fca2d4fae9fe0e3fd6d6d1345d55255d6df78 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCache.java 
4dd3826a67dfff66ce9c90027d61a9012c0a15e8 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCacheImpl.java 
62d7e5534486b53634de332875c5fd5d336c29b4 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/SerDeLowLevelCacheImpl.java
 2a39d2d32807a51346baad28b04d87670381b6d5 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/SimpleBufferManager.java 
41855e171eaa5bf8da638bc62bce3d0d49dc4bae 
  llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
c63ee5f79b4f9fc356f033960e0af1a7b0058038 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapRecordReader.java
 1378a01f44ef774a15f769460833064c6305b2d6 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/OrcColumnVectorProducer.java
 2a0c5ca92f3c7431f3c399f309a538f47eb27597 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/encoded/OrcEncodedDataReader.java
 85a42f945624c3ca468790772f52363b4064d8fc 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/encoded/SerDeEncodedDataReader.java
 d414b1405b7672767196b3eaad02baa516169288 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/MetadataCache.java 
8400fe98411ed07bd525a51a223fc35423136efb 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/OrcFileEstimateErrors.java
 30dc1b9da2002689b8b1917f46ae3ca24194f3be 
  
llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestCacheContentsTracker.java
 PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/llap/LlapCacheAwareFs.java 
af04a51b5536550b2d2f7d3e008cf2b2dea607d4 
  ql/src/java/org/apache/hadoop/hive/llap/LlapHiveUtils.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/EncodedReaderImpl.java 
241a3001e6e0002377736d6d0e820fde004b0bac 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/Reader.java 
210c987b7f580dacda5bdb487af9cf234a738b79 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/ReaderImpl.java 
a9a9f101948a970e0dbf2f77eeb6f688a88d1cbd 
  
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/VectorizedParquetRecordReader.java
 61e2556b08fe4247f35673f24378505ada20a605 
  storage-api/src/java/org/apache/hadoop/hive/common/io/CacheTag.java 
PRE-CREATION 
  storage-api/src/java/org/apache/hadoop/hive/common/io/DataCache.java 
2ac0a18a5026e76e65c3c3a8b81d5a844c472ed2 
  storage-api/src/java/org/apache/hadoop/hive/common/io/FileMetadataCache.java 
d7de3619380d24a1aeea2bac9a66485d7d468517 


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

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


Testing
---


Thanks,

Adam Szita

Re: Review Request 71575: HIVE-22284: Improve LLAP CacheContentsTracker to collect and display correct statistics

2019-10-03 Thread Adam Szita via Review Board


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

(Updated Oct. 3, 2019, 3:20 p.m.)


Review request for hive.


Changes
---

Added test, fixed findbugs and checkstyle errors


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


Repository: hive-git


Description
---

When keeping track of which buffers correspond to what Hive objects, 
CacheContentsTracker relies on cache tags.

Currently a tag is a simple String that ideally holds DB and table name, and a 
partition spec concatenated by . and / . The information here is derived from 
the Path of the file that is getting cached. Needless to say sometimes this 
produces a wrong tag especially for external tables.

Also there's a bug when calculating aggregated stats for a 'parent' tag 
(corresponding to the table of the partition) because the overall maxCount and 
maxSize do not add up to the sum of those in the partitions. This happens when 
buffers get removed from the cache.


Diffs (updated)
-

  llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java 
a351a193c6bc558bb420049c54b7657cd7d04b7c 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/CacheContentsTracker.java
 64c0125833af100fd7012b9751d075ab536ad1b0 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapCacheableBuffer.java 
f91a5d91a5b739dcbee98a1485ad4c59f6a9057b 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapDataBuffer.java 
405fca2d4fae9fe0e3fd6d6d1345d55255d6df78 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCache.java 
4dd3826a67dfff66ce9c90027d61a9012c0a15e8 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCacheImpl.java 
62d7e5534486b53634de332875c5fd5d336c29b4 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/SerDeLowLevelCacheImpl.java
 2a39d2d32807a51346baad28b04d87670381b6d5 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/SimpleBufferManager.java 
41855e171eaa5bf8da638bc62bce3d0d49dc4bae 
  llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
c63ee5f79b4f9fc356f033960e0af1a7b0058038 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapRecordReader.java
 1378a01f44ef774a15f769460833064c6305b2d6 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/OrcColumnVectorProducer.java
 2a0c5ca92f3c7431f3c399f309a538f47eb27597 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/encoded/OrcEncodedDataReader.java
 85a42f945624c3ca468790772f52363b4064d8fc 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/encoded/SerDeEncodedDataReader.java
 d414b1405b7672767196b3eaad02baa516169288 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/MetadataCache.java 
8400fe98411ed07bd525a51a223fc35423136efb 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/OrcFileEstimateErrors.java
 30dc1b9da2002689b8b1917f46ae3ca24194f3be 
  
llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestCacheContentsTracker.java
 PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/llap/LlapCacheAwareFs.java 
af04a51b5536550b2d2f7d3e008cf2b2dea607d4 
  ql/src/java/org/apache/hadoop/hive/llap/LlapHiveUtils.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/EncodedReaderImpl.java 
241a3001e6e0002377736d6d0e820fde004b0bac 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/Reader.java 
210c987b7f580dacda5bdb487af9cf234a738b79 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/ReaderImpl.java 
a9a9f101948a970e0dbf2f77eeb6f688a88d1cbd 
  
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/VectorizedParquetRecordReader.java
 61e2556b08fe4247f35673f24378505ada20a605 
  storage-api/src/java/org/apache/hadoop/hive/common/io/CacheTag.java 
PRE-CREATION 
  storage-api/src/java/org/apache/hadoop/hive/common/io/DataCache.java 
2ac0a18a5026e76e65c3c3a8b81d5a844c472ed2 
  storage-api/src/java/org/apache/hadoop/hive/common/io/FileMetadataCache.java 
d7de3619380d24a1aeea2bac9a66485d7d468517 


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

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


Testing
---


Thanks,

Adam Szita

Review Request 71575: HIVE-22284: Improve LLAP CacheContentsTracker to collect and display correct statistics

2019-10-02 Thread Adam Szita via Review Board


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

Review request for hive.


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


Repository: hive-git


Description
---

When keeping track of which buffers correspond to what Hive objects, 
CacheContentsTracker relies on cache tags.

Currently a tag is a simple String that ideally holds DB and table name, and a 
partition spec concatenated by . and / . The information here is derived from 
the Path of the file that is getting cached. Needless to say sometimes this 
produces a wrong tag especially for external tables.

Also there's a bug when calculating aggregated stats for a 'parent' tag 
(corresponding to the table of the partition) because the overall maxCount and 
maxSize do not add up to the sum of those in the partitions. This happens when 
buffers get removed from the cache.


Diffs
-

  llap-common/src/java/org/apache/hadoop/hive/llap/LlapUtil.java 
a351a193c6bc558bb420049c54b7657cd7d04b7c 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/CacheContentsTracker.java
 64c0125833af100fd7012b9751d075ab536ad1b0 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapCacheableBuffer.java 
f91a5d91a5b739dcbee98a1485ad4c59f6a9057b 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapDataBuffer.java 
405fca2d4fae9fe0e3fd6d6d1345d55255d6df78 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCache.java 
4dd3826a67dfff66ce9c90027d61a9012c0a15e8 
  llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCacheImpl.java 
62d7e5534486b53634de332875c5fd5d336c29b4 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/SerDeLowLevelCacheImpl.java
 2a39d2d32807a51346baad28b04d87670381b6d5 
  
llap-server/src/java/org/apache/hadoop/hive/llap/cache/SimpleBufferManager.java 
41855e171eaa5bf8da638bc62bce3d0d49dc4bae 
  llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java 
c63ee5f79b4f9fc356f033960e0af1a7b0058038 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapRecordReader.java
 1378a01f44ef774a15f769460833064c6305b2d6 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/decode/OrcColumnVectorProducer.java
 2a0c5ca92f3c7431f3c399f309a538f47eb27597 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/encoded/OrcEncodedDataReader.java
 85a42f945624c3ca468790772f52363b4064d8fc 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/encoded/SerDeEncodedDataReader.java
 d414b1405b7672767196b3eaad02baa516169288 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/MetadataCache.java 
8400fe98411ed07bd525a51a223fc35423136efb 
  
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/OrcFileEstimateErrors.java
 30dc1b9da2002689b8b1917f46ae3ca24194f3be 
  ql/src/java/org/apache/hadoop/hive/llap/LlapCacheAwareFs.java 
af04a51b5536550b2d2f7d3e008cf2b2dea607d4 
  ql/src/java/org/apache/hadoop/hive/llap/LlapHiveUtils.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/EncodedReaderImpl.java 
241a3001e6e0002377736d6d0e820fde004b0bac 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/Reader.java 
210c987b7f580dacda5bdb487af9cf234a738b79 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/encoded/ReaderImpl.java 
a9a9f101948a970e0dbf2f77eeb6f688a88d1cbd 
  
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/vector/VectorizedParquetRecordReader.java
 61e2556b08fe4247f35673f24378505ada20a605 
  storage-api/src/java/org/apache/hadoop/hive/common/io/CacheTag.java 
PRE-CREATION 
  storage-api/src/java/org/apache/hadoop/hive/common/io/DataCache.java 
2ac0a18a5026e76e65c3c3a8b81d5a844c472ed2 
  storage-api/src/java/org/apache/hadoop/hive/common/io/FileMetadataCache.java 
d7de3619380d24a1aeea2bac9a66485d7d468517 


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


Testing
---


Thanks,

Adam Szita

Re: Review Request 70453: HIVE-21584 Java 11 preparation: system class loader is not URLClassLoader

2019-04-12 Thread Adam Szita via Review Board


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




ql/src/java/org/apache/hadoop/hive/ql/exec/AddToClassPathAction.java
Lines 31 (patched)


do the _same_ vs do the _save_



ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java
Lines 2104 (patched)


Is it an expected scenario? Should we log a warning in this case or even 
throw an Exception instead?



ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkRecordHandler.java
Lines 81 (patched)


Can we refactor this method into a public static method in hive-common or 
even inside ql (within a utility class or something) as it is repeated 3 times.


- Adam Szita


On April 12, 2019, 9:50 a.m., Zoltan Matyus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70453/
> ---
> 
> (Updated April 12, 2019, 9:50 a.m.)
> 
> 
> Review request for hive, Zoltan Haindrich, Laszlo Pinter, and Adam Szita.
> 
> 
> Bugs: HIVE-21584
> https://issues.apache.org/jira/browse/HIVE-21584
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-21584 Java 11 preparation: system class loader is not URLClassLoader
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> f4dd586e1127e3dad56de856b91ba00a0f777ac2 
>   common/src/java/org/apache/hadoop/hive/common/JavaUtils.java 
> c011cd1626d608d5a3c8c950eddf96b46473d796 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/FunctionLocalizer.java
>  2a6ef3a2461fb4e4331da678ac8521135f304018 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/AddToClassPathAction.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 
> 36bc08f34e0aa24af68377181ccfb91c8635ddc5 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 
> 01dd93c5273caed17931d057e6d844ce17a511c5 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecMapper.java 
> 91868a46670f2f27dcd8f944df7c1cfca2faff32 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecReducer.java 
> e106bc9149832d8e7b1f0ecf5a9c0fdc172c7413 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkRecordHandler.java 
> f7ea212cfb9f0f1c2eb1895223fba147acd18cb8 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/RecordProcessor.java 
> 0ec7a04ce7a219f3a48a38f16d4626e8f2fc87b3 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> de5cd8b992c1d1fcc52611484cd6aa787c469bee 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/TestAddToClassPathAction.java 
> PRE-CREATION 
>   
> spark-client/src/main/java/org/apache/hive/spark/client/SparkClientUtilities.java
>  b434d8f7b7a3b585183cd842bd9893d00a85da1b 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  0642b39f58966b15d07e4163900919f1fad360cb 
> 
> 
> Diff: https://reviews.apache.org/r/70453/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zoltan Matyus
> 
>

Re: Review Request 69560: HIVE-21035: Race condition in SparkUtilities#getSparkSession

2018-12-13 Thread Adam Szita via Review Board


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


Ship it!




Ship It!

- Adam Szita


On Dec. 12, 2018, 3:13 p.m., Antal Sinkovits wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69560/
> ---
> 
> (Updated Dec. 12, 2018, 3:13 p.m.)
> 
> 
> Review request for hive, Denys Kuzmenko, Peter Vary, and Adam Szita.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> It can happen, that when in one given session, multiple queries are executed, 
> that due to a race condition, multiple spark application master gets kicked 
> off.
> In this case, the one that started earlier, will not be killed, when the hive 
> session closes, consuming resources.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkUtilities.java 
> d384ed6db6f4630ee2ef309854236e8f12926688 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/spark/TestSparkUtilities.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69560/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Antal Sinkovits
> 
>

Re: Review Request 69410: HIVE-20330: HCatLoader cannot handle multiple InputJobInfo objects for a job with multiple inputs

2018-11-23 Thread Adam Szita via Review Board



> On Nov. 22, 2018, 1:25 p.m., Peter Vary wrote:
> > My only concen is that some other components might use HCAT_KEY_JOB_INFO 
> > property values as well? Was this a public property key?
> > 
> > Otherwise nicely done!

Thanks for looking into this change, AFAIK HCAT_KEY_JOB_INFO is only used in 
HCatalog code internally, i.e. Pig is using the proper methods provided by 
LoadMetadata interface, and receives the input size numbers through that. It 
does not depend on job conf objects populated with value for HCAT_KEY_JOB_INFO.


- Adam


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


On Nov. 20, 2018, 12:53 p.m., Adam Szita wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69410/
> ---
> 
> (Updated Nov. 20, 2018, 12:53 p.m.)
> 
> 
> Review request for hive, Nandor Kollar and Peter Vary.
> 
> 
> Bugs: HIVE-20330
> https://issues.apache.org/jira/browse/HIVE-20330
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The change in this patch is that we're not just serializing and putting one 
> InputJobInfo into JobConf, but rather always append to a list (or create it 
> on the first occurrence) of InputJobInfo instances in it.
> This ensures that if multiple tables serve as inputs in a job, Pig can 
> retrieve information for each of the tables, not just the last one added.
> 
> I've also discovered a bug in InputJobInfo.writeObject() where the 
> ObjectOutputStream was closed by mistake after writing partition information 
> in a compressed manner. Closing the compressed writer inevitably closed the 
> OOS on the context and prevented any other objects to be written into OOS - I 
> had to fix that because it prevented serializing InputJobInfo instances 
> inside a list.
> 
> 
> Diffs
> -
> 
>   hcatalog/core/src/main/java/org/apache/hive/hcatalog/common/HCatUtil.java 
> 8e72a1275a5cdcc2d778080fff6bb82198395f5f 
>   
> hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FosterStorageHandler.java
>  195eaa367933990e3ef0ef879f34049c65822aee 
>   
> hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/HCatBaseInputFormat.java
>  8d7a8f9df9412105ec7d77fad9af0d7dd18f4323 
>   
> hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/HCatInputFormat.java
>  ad6f3eb9f93338023863c6239d6af0449b20ff9c 
>   
> hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/InitializeInput.java
>  364382d9ccf6eb9fc29689b0eb5f973f422051b4 
>   
> hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/InputJobInfo.java
>  ac1dd54be821d32aa008d41514df05a41f16223c 
>   
> hcatalog/core/src/test/java/org/apache/hive/hcatalog/common/TestHCatUtil.java 
> 91aa4fa2693e0b0bd65c1667210af340619f552d 
>   
> hcatalog/hcatalog-pig-adapter/src/main/java/org/apache/hive/hcatalog/pig/HCatLoader.java
>  c3bde2d2a3cbd09fb0b1ed758bf4f2b1041a23cb 
>   
> hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/AbstractHCatLoaderTest.java
>  58981f88ef6abfbf7a4b7ffc3116c53d47e86fde 
> 
> 
> Diff: https://reviews.apache.org/r/69410/diff/1/
> 
> 
> Testing
> ---
> 
> Added (true) unit tests to verify my method of adding/retrieving InputJobInfo 
> instances to/from config instances.
> Added (integration-like) unit tests to mock Pig calling HCatLoader for 
> multiple input tables, and checking the reported input sizes.
> 
> 
> Thanks,
> 
> Adam Szita
> 
>

Review Request 69410: HIVE-20330: HCatLoader cannot handle multiple InputJobInfo objects for a job with multiple inputs

2018-11-20 Thread Adam Szita via Review Board


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

Review request for hive, Nandor Kollar and Peter Vary.


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


Repository: hive-git


Description
---

The change in this patch is that we're not just serializing and putting one 
InputJobInfo into JobConf, but rather always append to a list (or create it on 
the first occurrence) of InputJobInfo instances in it.
This ensures that if multiple tables serve as inputs in a job, Pig can retrieve 
information for each of the tables, not just the last one added.

I've also discovered a bug in InputJobInfo.writeObject() where the 
ObjectOutputStream was closed by mistake after writing partition information in 
a compressed manner. Closing the compressed writer inevitably closed the OOS on 
the context and prevented any other objects to be written into OOS - I had to 
fix that because it prevented serializing InputJobInfo instances inside a list.


Diffs
-

  hcatalog/core/src/main/java/org/apache/hive/hcatalog/common/HCatUtil.java 
8e72a1275a5cdcc2d778080fff6bb82198395f5f 
  
hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FosterStorageHandler.java
 195eaa367933990e3ef0ef879f34049c65822aee 
  
hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/HCatBaseInputFormat.java
 8d7a8f9df9412105ec7d77fad9af0d7dd18f4323 
  
hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/HCatInputFormat.java
 ad6f3eb9f93338023863c6239d6af0449b20ff9c 
  
hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/InitializeInput.java
 364382d9ccf6eb9fc29689b0eb5f973f422051b4 
  
hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/InputJobInfo.java
 ac1dd54be821d32aa008d41514df05a41f16223c 
  hcatalog/core/src/test/java/org/apache/hive/hcatalog/common/TestHCatUtil.java 
91aa4fa2693e0b0bd65c1667210af340619f552d 
  
hcatalog/hcatalog-pig-adapter/src/main/java/org/apache/hive/hcatalog/pig/HCatLoader.java
 c3bde2d2a3cbd09fb0b1ed758bf4f2b1041a23cb 
  
hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/AbstractHCatLoaderTest.java
 58981f88ef6abfbf7a4b7ffc3116c53d47e86fde 


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


Testing
---

Added (true) unit tests to verify my method of adding/retrieving InputJobInfo 
instances to/from config instances.
Added (integration-like) unit tests to mock Pig calling HCatLoader for multiple 
input tables, and checking the reported input sizes.


Thanks,

Adam Szita

Re: Review Request 68656: HIVE-20505: upgrade org.openjdk.jmh:jmh-core to 1.21

2018-09-07 Thread Adam Szita via Review Board


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


Ship it!




Ship It!

- Adam Szita


On Sept. 6, 2018, 10:07 a.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68656/
> ---
> 
> (Updated Sept. 6, 2018, 10:07 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20505: upgrade org.openjdk.jmh:jmh-core to 1.21
> 
> 
> Diffs
> -
> 
>   itests/hive-jmh/pom.xml 0abefdf791a04593c547119256a755adcd78bda5 
> 
> 
> Diff: https://reviews.apache.org/r/68656/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 67895: Improve HiveMetaStoreClient.dropDatabase

2018-07-13 Thread Adam Szita via Review Board


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

(Updated July 13, 2018, 9:22 a.m.)


Review request for hive.


Changes
---

Rebased to top of master again...


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


Repository: hive-git


Description
---

HiveMetaStoreClient.dropDatabase has a strange implementation to ensure dealing 
with client side hooks (for non-native tables e.g. HBase). Currently it starts 
by retrieving all the tables from HMS, and then sends dropTable calls to HMS 
table-by-table. At the end a dropDatabase just to be sure  

I believe this could be refactored so that it speeds up the dropDB in 
situations where the average table count per DB is very high.


Diffs (updated)
-

  hbase-handler/src/test/queries/positive/drop_database_table_hooks.q 
PRE-CREATION 
  hbase-handler/src/test/results/positive/drop_database_table_hooks.q.out 
PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/metadata/TableIterable.java 
d8e771d0ffa7d680b2a22436727f896674cd40ff 
  ql/src/test/org/apache/hadoop/hive/ql/metadata/TestTableIterable.java 
6637d150b84c9fa86e6a3a90449606437e7c9d72 
  
service/src/java/org/apache/hive/service/cli/operation/GetColumnsOperation.java 
838dd89ca82792ca8af8eb0f30aa63e690e41f43 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 8d88749effa89e50d8be8ed216419cd77836fd34 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
 bfd7141a8b987e5288277a46d56de32574d9aa69 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/TableIterable.java
 PRE-CREATION 
  
standalone-metastore/metastore-common/src/test/java/org/apache/hadoop/hive/metastore/TestTableIterable.java
 PRE-CREATION 


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

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


Testing
---

Drop database is an existing feature - existing tests should be fine, but since 
I'm poking around client side hooks I've added an HBase drop db qtest so that 
code path is covered


Thanks,

Adam Szita

Review Request 67895: Improve HiveMetaStoreClient.dropDatabase

2018-07-12 Thread Adam Szita via Review Board


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

Review request for hive.


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


Repository: hive-git


Description
---

HiveMetaStoreClient.dropDatabase has a strange implementation to ensure dealing 
with client side hooks (for non-native tables e.g. HBase). Currently it starts 
by retrieving all the tables from HMS, and then sends dropTable calls to HMS 
table-by-table. At the end a dropDatabase just to be sure  

I believe this could be refactored so that it speeds up the dropDB in 
situations where the average table count per DB is very high.


Diffs
-

  hbase-handler/src/test/queries/positive/drop_database_table_hooks.q 
PRE-CREATION 
  hbase-handler/src/test/results/positive/drop_database_table_hooks.q.out 
PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/metadata/TableIterable.java 
d8e771d0ffa7d680b2a22436727f896674cd40ff 
  ql/src/test/org/apache/hadoop/hive/ql/metadata/TestTableIterable.java 
6637d150b84c9fa86e6a3a90449606437e7c9d72 
  
service/src/java/org/apache/hive/service/cli/operation/GetColumnsOperation.java 
838dd89ca82792ca8af8eb0f30aa63e690e41f43 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 8d88749effa89e50d8be8ed216419cd77836fd34 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
 bfd7141a8b987e5288277a46d56de32574d9aa69 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/TableIterable.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/TestTableIterable.java
 PRE-CREATION 


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


Testing
---

Drop database is an existing feature - existing tests should be fine, but since 
I'm poking around client side hooks I've added an HBase drop db qtest so that 
code path is covered


Thanks,

Adam Szita

Re: Review Request 65731: HIVE-18699: Check for duplicate partitions in HiveMetastore.exchange_partitions

2018-02-22 Thread Adam Szita via Review Board


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


Ship it!




Ship It!

- Adam Szita


On Feb. 21, 2018, 11:37 a.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65731/
> ---
> 
> (Updated Feb. 21, 2018, 11:37 a.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18699
> https://issues.apache.org/jira/browse/HIVE-18699
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Extended the HiveMetastore.exchange_partitions method to check if the 
> partitions to be exchanged don't exist in the dest table. If one of the 
> partitions already exists, throw a MetaException with a proper error message.
> 
> Previously an exception like this (wrapped in a MetaException) was thrown:
> Insert of object
> "org.apache.hadoop.hive.metastore.model.MPartition@4e78fff5" using statement 
> "INSERT INTO PARTITIONS
> (PART_ID,CREATE_TIME,LAST_ACCESS_TIME,PART_NAME,SD_ID,TBL_ID) VALUES 
> (?,?,?,?,?,?)" failed : The statement was
> aborted because it would have caused a duplicate key value in a unique or 
> primary key constraint or unique index
> identified by 'UNIQUEPARTITION' defined on 'PARTITIONS'.
> 
> From user point of view, the type of the exception is not changed 
> (MetaException), just the error message is changed to a more understandable 
> one.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  47de215 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestExchangePartitions.java
>  3a06aec 
> 
> 
> Diff: https://reviews.apache.org/r/65731/diff/1/
> 
> 
> Testing
> ---
> 
> Tests already exist for this use case in TestExchangePartitions:
> - testExchangePartitionsPartAlreadyExists
> - testExchangePartitionPartAlreadyExists
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65731: HIVE-18699: Check for duplicate partitions in HiveMetastore.exchange_partitions

2018-02-22 Thread Adam Szita via Review Board



> On Feb. 21, 2018, 1:37 p.m., Peter Vary wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 3370 (patched)
> > 
> >
> > How "expensive" is this call? Is this a simple query? What happens if 
> > the destintaion table has 1m partitions? :)

I don't see any other way (currently available) that is more lightweight than 
this is. Under the hood this calls getPartitionNamesNoTxn which executes an 
actual "select parititionName from .." statement.


- Adam


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


On Feb. 21, 2018, 11:37 a.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65731/
> ---
> 
> (Updated Feb. 21, 2018, 11:37 a.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18699
> https://issues.apache.org/jira/browse/HIVE-18699
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Extended the HiveMetastore.exchange_partitions method to check if the 
> partitions to be exchanged don't exist in the dest table. If one of the 
> partitions already exists, throw a MetaException with a proper error message.
> 
> Previously an exception like this (wrapped in a MetaException) was thrown:
> Insert of object
> "org.apache.hadoop.hive.metastore.model.MPartition@4e78fff5" using statement 
> "INSERT INTO PARTITIONS
> (PART_ID,CREATE_TIME,LAST_ACCESS_TIME,PART_NAME,SD_ID,TBL_ID) VALUES 
> (?,?,?,?,?,?)" failed : The statement was
> aborted because it would have caused a duplicate key value in a unique or 
> primary key constraint or unique index
> identified by 'UNIQUEPARTITION' defined on 'PARTITIONS'.
> 
> From user point of view, the type of the exception is not changed 
> (MetaException), just the error message is changed to a more understandable 
> one.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  47de215 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestExchangePartitions.java
>  3a06aec 
> 
> 
> Diff: https://reviews.apache.org/r/65731/diff/1/
> 
> 
> Testing
> ---
> 
> Tests already exist for this use case in TestExchangePartitions:
> - testExchangePartitionsPartAlreadyExists
> - testExchangePartitionPartAlreadyExists
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65507: HIVE-18580: Create tests to cover exchange partitions

2018-02-07 Thread Adam Szita via Review Board


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


Fix it, then Ship it!




Thanks for the patch Marta, this is incredibly thourough!


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestExchangePartitions.java
Lines 1480-1501 (patched)


I'd mark these helper methods as static


- Adam Szita


On Feb. 5, 2018, 6:01 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65507/
> ---
> 
> (Updated Feb. 5, 2018, 6:01 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18580
> https://issues.apache.org/jira/browse/HIVE-18580
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - int Partition exchange_partition(Map, String, String, 
> String, String)
> - List Partition exchange_partition(Map, String, 
> String, String, String)
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestExchangePartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65507/diff/1/
> 
> 
> Testing
> ---
> 
> Ran the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65353: Create tests to cover getTableMeta method

2018-01-30 Thread Adam Szita via Review Board


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

(Updated Jan. 30, 2018, 4:53 p.m.)


Review request for hive, Marta Kuczora and Peter Vary.


Changes
---

Addressed review comments


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


Repository: hive-git


Description
---

Create tests to cover getTableMeta method


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetTableMeta.java
 PRE-CREATION 


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

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


Testing
---

Have run the tests themselves


Thanks,

Adam Szita

Re: Review Request 65380: HIVE-18566: Create tests to cover adding partitions from PartitionSpec

2018-01-30 Thread Adam Szita via Review Board


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


Ship it!




Ship It!

- Adam Szita


On Jan. 30, 2018, 8:15 a.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65380/
> ---
> 
> (Updated Jan. 30, 2018, 8:15 a.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18566
> https://issues.apache.org/jira/browse/HIVE-18566
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> he following methods of IMetaStoreClient are covered by this test.
> - int add_partitions_pspec(PartitionSpecProxy)
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65380/diff/2/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65380: HIVE-18566: Create tests to cover adding partitions from PartitionSpec

2018-01-29 Thread Adam Szita via Review Board


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



Nice patch with well covered cases.
Since this is a lot of test code, wouldn't it make sense to move the 
PartitionSpec-related code into a separate class (rather than having a 2k+ LOC 
class)?


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 2220-2221 (patched)


123456 may be a contant to be considered for extraction


- Adam Szita


On Jan. 29, 2018, 12:27 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65380/
> ---
> 
> (Updated Jan. 29, 2018, 12:27 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18566
> https://issues.apache.org/jira/browse/HIVE-18566
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> he following methods of IMetaStoreClient are covered by this test.
> - int add_partitions_pspec(PartitionSpecProxy)
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
>  09be321 
> 
> 
> Diff: https://reviews.apache.org/r/65380/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65349: HIVE-18544: Create tests to cover methods for appending Partitions

2018-01-29 Thread Adam Szita via Review Board


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


Ship it!




Ship It!

- Adam Szita


On Jan. 29, 2018, 12:49 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65349/
> ---
> 
> (Updated Jan. 29, 2018, 12:49 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18544
> https://issues.apache.org/jira/browse/HIVE-18544
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - Partition appendPartition(String, String, List)
> - Partition appendPartition(String, String, String)
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAppendPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65349/diff/2/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65349: HIVE-18544: Create tests to cover methods for appending Partitions

2018-01-29 Thread Adam Szita via Review Board


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



Thanks for the patch Marta, it looks very thourough! I've added my 2 cents: 
small observations regarding helper methods only.


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAppendPartitions.java
Lines 489-495 (patched)


When defining table types we should rely on the enum values IMHO: 
TableType.EXTERNAL_TABLE.name()



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAppendPartitions.java
Lines 536-539 (patched)


Another way of doing this is:

Arrays.stream(input.split("/")).map(v->v.split("=")[1]).collect(toList())


- Adam Szita


On Jan. 26, 2018, 1:03 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65349/
> ---
> 
> (Updated Jan. 26, 2018, 1:03 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18544
> https://issues.apache.org/jira/browse/HIVE-18544
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - Partition appendPartition(String, String, List)
> - Partition appendPartition(String, String, String)
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAppendPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65349/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Review Request 65353: Create tests to cover getTableMeta method

2018-01-26 Thread Adam Szita via Review Board


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

Review request for hive, Marta Kuczora and Peter Vary.


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


Repository: hive-git


Description
---

Create tests to cover getTableMeta method


Diffs
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetTableMeta.java
 PRE-CREATION 


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


Testing
---

Have run the tests themselves


Thanks,

Adam Szita

Re: Review Request 65217: Create tests to cover alterPartition and renamePartition methods

2018-01-25 Thread Adam Szita via Review Board


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

(Updated Jan. 25, 2018, 1:52 p.m.)


Review request for hive, Marta Kuczora and Peter Vary.


Changes
---

Checkstyle fixes


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


Repository: hive-git


Description
---

Adding IMetastoreClient API tests that cover alterPartition ande 
renamePartition methods


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/65217/diff/4/

Changes: https://reviews.apache.org/r/65217/diff/3-4/


Testing
---


Thanks,

Adam Szita

Re: Review Request 65285: Create tests to cover getPartition(s) methods

2018-01-25 Thread Adam Szita via Review Board


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

(Updated Jan. 25, 2018, 1:51 p.m.)


Review request for hive, Marta Kuczora and Peter Vary.


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


Repository: hive-git


Description
---

Create tests to cover getPartition(s) methods


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetPartitions.java
 PRE-CREATION 


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

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


Testing
---


Thanks,

Adam Szita

Re: Review Request 65284: Create tests to cover listPartition(s) methods

2018-01-25 Thread Adam Szita via Review Board



> On Jan. 25, 2018, 11:37 a.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestListPartitions.java
> > Lines 762-765 (patched)
> > 
> >
> > Is this the same use case as in the 
> > testListPartitionsByFilterEmptyFilter test? If it is, I think it is enough 
> > to have only one test for this use case.

Removed redundant case


> On Jan. 25, 2018, 11:37 a.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestListPartitions.java
> > Lines 1028-1031 (patched)
> > 
> >
> > What is the expected behaviour in this case? Should the 
> > getNumPartitionsByFilter return 0 or an other value?

Added missing assertion


- Adam


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


On Jan. 25, 2018, 1:47 p.m., Adam Szita wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65284/
> ---
> 
> (Updated Jan. 25, 2018, 1:47 p.m.)
> 
> 
> Review request for hive, Marta Kuczora and Peter Vary.
> 
> 
> Bugs: HIVE-18484
> https://issues.apache.org/jira/browse/HIVE-18484
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Create tests to cover listPartition(s) methods
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestListPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65284/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam Szita
> 
>

Re: Review Request 65284: Create tests to cover listPartition(s) methods

2018-01-25 Thread Adam Szita via Review Board


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

(Updated Jan. 25, 2018, 1:47 p.m.)


Review request for hive, Marta Kuczora and Peter Vary.


Changes
---

Addressing Marta's comments


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


Repository: hive-git


Description
---

Create tests to cover listPartition(s) methods


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestListPartitions.java
 PRE-CREATION 


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

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


Testing
---


Thanks,

Adam Szita

Re: Review Request 65285: Create tests to cover getPartition(s) methods

2018-01-24 Thread Adam Szita via Review Board


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

(Updated Jan. 24, 2018, 2:02 p.m.)


Review request for hive, Marta Kuczora and Peter Vary.


Changes
---

Addressed review comments


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


Repository: hive-git


Description
---

Create tests to cover getPartition(s) methods


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetPartitions.java
 PRE-CREATION 


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

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


Testing
---


Thanks,

Adam Szita

Re: Review Request 65285: Create tests to cover getPartition(s) methods

2018-01-24 Thread Adam Szita via Review Board



> On Jan. 23, 2018, 4:12 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetPartitions.java
> > Lines 200 (patched)
> > 
> >
> > Maybe test for case insensitivity?

Added test case


> On Jan. 23, 2018, 4:12 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetPartitions.java
> > Lines 236 (patched)
> > 
> >
> > Maybe test for no db?

Added test case


> On Jan. 23, 2018, 4:12 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetPartitions.java
> > Lines 305 (patched)
> > 
> >
> > Maybe test for no database?

Added test case


> On Jan. 23, 2018, 4:12 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetPartitions.java
> > Lines 347 (patched)
> > 
> >
> > Maybe test for case insensitivity?

Added test case


- Adam


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


On Jan. 24, 2018, 2:02 p.m., Adam Szita wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65285/
> ---
> 
> (Updated Jan. 24, 2018, 2:02 p.m.)
> 
> 
> Review request for hive, Marta Kuczora and Peter Vary.
> 
> 
> Bugs: HIVE-18483
> https://issues.apache.org/jira/browse/HIVE-18483
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Create tests to cover getPartition(s) methods
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65285/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam Szita
> 
>

Re: Review Request 65284: Create tests to cover listPartition(s) methods

2018-01-24 Thread Adam Szita via Review Board


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

(Updated Jan. 24, 2018, 11:22 a.m.)


Review request for hive, Marta Kuczora and Peter Vary.


Changes
---

Addressed review comments


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


Repository: hive-git


Description
---

Create tests to cover listPartition(s) methods


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestListPartitions.java
 PRE-CREATION 


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

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


Testing
---


Thanks,

Adam Szita

Re: Review Request 65210: HIVE-18480 Create tests for function related methods

2018-01-23 Thread Adam Szita via Review Board


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


Ship it!




Ship It!

- Adam Szita


On Jan. 23, 2018, 4:47 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> ---
> 
> (Updated Jan. 23, 2018, 4:47 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The test tries to go throught all af the function related tests and test with 
> various imputs, so not only the happy path, but the edge cases are covered too
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65210/diff/3/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65264: HIVE-18481: Create tests for table related methods (get, list, exists)

2018-01-23 Thread Adam Szita via Review Board


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


Ship it!




Ship It!

- Adam Szita


On Jan. 23, 2018, 11:18 a.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65264/
> ---
> 
> (Updated Jan. 23, 2018, 11:18 a.m.)
> 
> 
> Review request for hive, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
> 
> 
> Bugs: HIVE-18481
> https://issues.apache.org/jira/browse/HIVE-18481
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Create IMetaStoreClient tests to cover the table query methods
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesGetListExists.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65264/diff/3/
> 
> 
> Testing
> ---
> 
> Run the created tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65265: HIVE-18509 Create tests for table manipulation related methods (create, alter, drop)

2018-01-23 Thread Adam Szita via Review Board


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


Fix it, then Ship it!




Looking good, thanks Peter!


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java
Lines 234-235 (patched)


Could use assertEquals here instead IMHO



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java
Lines 429 (patched)


Is this line of code necessary here?


- Adam Szita


On Jan. 23, 2018, 11:48 a.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65265/
> ---
> 
> (Updated Jan. 23, 2018, 11:48 a.m.)
> 
> 
> Review request for hive, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
> 
> 
> Bugs: HIVE-18509
> https://issues.apache.org/jira/browse/HIVE-18509
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Added the new tests.
> Modified AbstractMetaStoreService, so when starting mini-metastore it is 
> possible to provide configuration value overrides for non-metastore config 
> values (like fs trash settings)
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  ed071f8 
> 
> 
> Diff: https://reviews.apache.org/r/65265/diff/3/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65210: HIVE-18480 Create tests for function related methods

2018-01-23 Thread Adam Szita via Review Board


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


Fix it, then Ship it!




Looks good Peter, thanks for the patch!


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Lines 103 (patched)


Should we extract "org.apache.hadoop.hive.ql.udf.generic.GenericUDFUpper" 
into a constant as it is used multiple times here?


- Adam Szita


On Jan. 22, 2018, 2:33 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> ---
> 
> (Updated Jan. 22, 2018, 2:33 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The test tries to go throught all af the function related tests and test with 
> various imputs, so not only the happy path, but the edge cases are covered too
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65210/diff/2/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65217: Create tests to cover alterPartition and renamePartition methods

2018-01-23 Thread Adam Szita via Review Board



> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 223 (patched)
> > 
> >
> > When altering a partition, is it possible to modify its 
> > StorageDescriptor and location? 
> > If so, what happens if we try to modify the sd or location to null or 
> > empty? 
> > Is it possible to change the columns in the partition's storage 
> > descriptor?

SD can be changed completely, I change location by hand and I also added a 
change for col list now, thanks!


> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 497 (patched)
> > 
> >
> > When tyring to alter two partitions, but an exception occurs during 
> > altering one of them, what happens with the other? All modification will be 
> > rolled back correctly?

Altered my test case to check this, it is rolled back correctly


> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 769 (patched)
> > 
> >
> > What happens if the newPart partition has different db/table than the 
> > ones set in the method parameter?

Added test case for this


> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 770 (patched)
> > 
> >
> > What happens if the db/table is null in the newPart partition?

Added test case for this


> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 771 (patched)
> > 
> >
> > Is it possible to change any other attribute of a partition besides the 
> > value with the rename method?

Added test case for this


- Adam


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


On Jan. 23, 2018, 1:38 p.m., Adam Szita wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65217/
> ---
> 
> (Updated Jan. 23, 2018, 1:38 p.m.)
> 
> 
> Review request for hive, Marta Kuczora and Peter Vary.
> 
> 
> Bugs: HIVE-18468
> https://issues.apache.org/jira/browse/HIVE-18468
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Adding IMetastoreClient API tests that cover alterPartition ande 
> renamePartition methods
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65217/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam Szita
> 
>

Re: Review Request 65217: Create tests to cover alterPartition and renamePartition methods

2018-01-23 Thread Adam Szita via Review Board


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

(Updated Jan. 23, 2018, 1:38 p.m.)


Review request for hive, Marta Kuczora and Peter Vary.


Changes
---

Added additional cases as per Marta's comments


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


Repository: hive-git


Description
---

Adding IMetastoreClient API tests that cover alterPartition ande 
renamePartition methods


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
 PRE-CREATION 


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

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


Testing
---


Thanks,

Adam Szita

Review Request 65285: Create tests to cover getPartition(s) methods

2018-01-23 Thread Adam Szita via Review Board


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

Review request for hive, Marta Kuczora and Peter Vary.


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


Repository: hive-git


Description
---

Create tests to cover getPartition(s) methods


Diffs
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetPartitions.java
 PRE-CREATION 


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


Testing
---


Thanks,

Adam Szita

Review Request 65284: Create tests to cover listPartition(s) methods

2018-01-23 Thread Adam Szita via Review Board


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

Review request for hive, Marta Kuczora and Peter Vary.


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


Repository: hive-git


Description
---

Create tests to cover listPartition(s) methods


Diffs
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestListPartitions.java
 PRE-CREATION 


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


Testing
---


Thanks,

Adam Szita

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index methods

2018-01-22 Thread Adam Szita via Review Board


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



Thanks for this patch Marta, it looks good! I've made a few recommendations only


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddAlterDropIndexes.java
Lines 678 (patched)


For handling future bug fixes more easily we could use consistent notation 
for these TODOs, see line 343.



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddAlterDropIndexes.java
Lines 876-880 (patched)


If you'd like to shorten this one, I can recommend something like this:

cols.stream().collect(
  Collectors.toMap(col->col.getName(), col->col)
).get("id")


- Adam Szita


On Jan. 19, 2018, 3:17 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65241/
> ---
> 
> (Updated Jan. 19, 2018, 3:17 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18496
> https://issues.apache.org/jira/browse/HIVE-18496
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - void createIndex(Index, Table)
> - boolean dropIndex(String, String, String, boolean)
> - void alter_index(String, String, String, Index)
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddAlterDropIndexes.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65241/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65240: HIVE-18498: Create tests to cover get and list index methods

2018-01-22 Thread Adam Szita via Review Board


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



This change looks good! I only spotted some small code-duplications that we may 
want to clean up.


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetListIndexes.java
Lines 312-329 (patched)


We may want to separate the common code parts into helper/checker methods 
here and then call with that with different num values.



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetListIndexes.java
Lines 463-481 (patched)


Same remark here relating code duplication


- Adam Szita


On Jan. 19, 2018, 3:05 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65240/
> ---
> 
> (Updated Jan. 19, 2018, 3:05 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18498
> https://issues.apache.org/jira/browse/HIVE-18498
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - Index getIndex(String, String, String)
> - List listIndexes(String, String, short)
> - List listIndexNames(String, String, short)
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetListIndexes.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65240/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65219: HIVE-18486: Create tests to cover methods for adding Partitions

2018-01-19 Thread Adam Szita via Review Board


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



Looks good! I added small issues to fix, similarly with the dropPartitions 
change: some helper methods could be static (the ones that don't depend on HMS 
client certainly) and some could use Lists.newArrayList


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 68 (patched)


Again this may be -1 instead, when MAX as a value is not used for any other 
reason than returning all partitions



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 672 (patched)


This is found multiple times in the code, should we create a method for it?


- Adam Szita


On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> ---
> 
> (Updated Jan. 18, 2018, 4:58 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List)
> - List add_partitions(List, boolean, boolean)
> 
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65219/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65217: Create tests to cover alterPartition and renamePartition methods

2018-01-19 Thread Adam Szita via Review Board



> On Jan. 19, 2018, 11:41 a.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 122 (patched)
> > 
> >
> > nit: Why static?

I like my helper methods static :)


> On Jan. 19, 2018, 11:41 a.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 150 (patched)
> > 
> >
> > nit: Can we stick to one notation? I do not like _ in function names :)

fixed


> On Jan. 19, 2018, 11:41 a.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 450 (patched)
> > 
> >
> > Question: Maybe in setup?

Some tests depend on not having a table in HMS.


- Adam


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


On Jan. 19, 2018, 2:29 p.m., Adam Szita wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65217/
> ---
> 
> (Updated Jan. 19, 2018, 2:29 p.m.)
> 
> 
> Review request for hive, Marta Kuczora and Peter Vary.
> 
> 
> Bugs: HIVE-18468
> https://issues.apache.org/jira/browse/HIVE-18468
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Adding IMetastoreClient API tests that cover alterPartition ande 
> renamePartition methods
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65217/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam Szita
> 
>

Re: Review Request 65217: Create tests to cover alterPartition and renamePartition methods

2018-01-19 Thread Adam Szita via Review Board


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

(Updated Jan. 19, 2018, 2:29 p.m.)


Review request for hive, Marta Kuczora and Peter Vary.


Changes
---

Fixed smaller issues


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


Repository: hive-git


Description
---

Adding IMetastoreClient API tests that cover alterPartition ande 
renamePartition methods


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
 PRE-CREATION 


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

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


Testing
---


Thanks,

Adam Szita

Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

2018-01-19 Thread Adam Szita via Review Board


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



Overall looks good, I just have small observations.

One more nitpick: some helper methods could be static e.g. getYearAndMonthCols


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
Lines 64 (patched)


Is there any reason to use 10? Supplying -1 would return all partitions 
which I think was the original intent in the test cases below (in 
listPartitions calls).



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
Lines 165 (patched)


createTable is called here twice because it's also in @Before. Does this 
mean it overrides the existing table with the supplied tableParams?



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
Lines 539-553 (patched)


Guava lib has a handy method for these use cases:

 ArrayList com.google.common.collect.Lists.newArrayList(E... elements)

...which is doing exactly the same + optimising on initial list size.


- Adam Szita


On Jan. 18, 2018, 1:11 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> ---
> 
> (Updated Jan. 18, 2018, 1:11 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List, boolean)
> - boolean dropPartition(String, String, List, PartitionDropOptions)
> - boolean dropPartition(String, String, String, boolean)
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65213/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Review Request 65217: Create tests to cover alterPartition and renamePartition methods

2018-01-18 Thread Adam Szita via Review Board


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

Review request for hive, Marta Kuczora and Peter Vary.


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


Repository: hive-git


Description
---

Adding IMetastoreClient API tests that cover alterPartition ande 
renamePartition methods


Diffs
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
 PRE-CREATION 


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


Testing
---


Thanks,

Adam Szita

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

2018-01-11 Thread Adam Szita via Review Board



> On Jan. 10, 2018, 6:35 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 350 (patched)
> > 
> >
> > I think we should catch the exception and assert the exception type is 
> > InvalidOperationException so that we catch errors like if someone changes 
> > the thrown exception in the future.
> 
> Peter Vary wrote:
> Shall I separate the test case into two? With/Without cascade? There is a 
> little extra stuff there creating the table/function/index in two tests which 
> I wanted to avoid with one test case. That is why I decied to keep the 
> cascade tests in one test case and used try-catch here to check the exception 
> without the cascade option, and then proceed with the test and drop the 
> database with the cascade option.
> 
> What do you think?

I'd vote for separation. One test case for cascade and one for without cascade. 
The latter should have an @Test(expected = ..) annotation IMHO


- Adam


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


On Jan. 11, 2018, 12:54 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 11, 2018, 12:54 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/4/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 63528: HIVE-17969: Metastore to alter table in batches of partitions when renaming table

2017-11-07 Thread Adam Szita via Review Board



> On Nov. 6, 2017, 6:51 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
> > Lines 263-264 (original), 275-278 (patched)
> > 
> >
> > Shouldn't the partValues contain only the values from partBatch here?

Good catch, thanks!


- Adam


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


On Nov. 7, 2017, 10:22 a.m., Adam Szita wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63528/
> ---
> 
> (Updated Nov. 7, 2017, 10:22 a.m.)
> 
> 
> Review request for hive, Peter Vary and Barna Zsombor Klara.
> 
> 
> Bugs: HIVE-17969
> https://issues.apache.org/jira/browse/HIVE-17969
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Refactoring alter table code to use batching of partitions when calling the 
> heavy removeUnusedColumnDescriptor method
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
>  ccadac1ada6aaae884ab39f5d99e91b8c542404e 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  7334a0c9fa87b1fe5b4f6c9d2073a477bc0f11ad 
> 
> 
> Diff: https://reviews.apache.org/r/63528/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam Szita
> 
>

Re: Review Request 63528: HIVE-17969: Metastore to alter table in batches of partitions when renaming table

2017-11-07 Thread Adam Szita via Review Board


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

(Updated Nov. 7, 2017, 10:22 a.m.)


Review request for hive, Peter Vary and Barna Zsombor Klara.


Changes
---

Adding missing null-check. Correcting issue pointed out by Vihang


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


Repository: hive-git


Description
---

Refactoring alter table code to use batching of partitions when calling the 
heavy removeUnusedColumnDescriptor method


Diffs (updated)
-

  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
 ccadac1ada6aaae884ab39f5d99e91b8c542404e 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 7334a0c9fa87b1fe5b4f6c9d2073a477bc0f11ad 


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

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


Testing
---


Thanks,

Adam Szita

Re: Review Request 63528: HIVE-17969: Metastore to alter table in batches of partitions when renaming table

2017-11-06 Thread Adam Szita via Review Board



> On Nov. 2, 2017, 10:40 p.m., Peter Vary wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
> > Lines 265 (patched)
> > 
> >
> > Shouldn't we use 
> > parts = msdb.getPartitions(dbname, name, partitionBatchSize);
> > instead
> > parts = msdb.getPartitions(dbname, name, -1);
> > 
> > This way we will load only the required amount of partitions, instead 
> > of all.
> 
> Adam Szita wrote:
> Although msdb.getPartitions(dbname, name, partitionBatchSize) only loads 
> partitionBatchSize amount of partitions, it will always return the same set 
> of partitions. This is used in dropPartitions and it's not a problem there as 
> we're also removing the requested set, so it is guaranteed to get new ones in 
> the next call. For our use case here it is sadly not feasable to use this 
> call. Taking a look at RawStore class I can see no iterative way to query the 
> partitions from the DB behind HMS.
> 
> Peter Vary wrote:
> Still think it would be useful, to have the possibility to load only 
> partial partition list into memory to decrease memory load. Shall we open a 
> follow-up jira for this?
> Thanks,
> Peter

Looks like a fine idea, I've opened HIVE-17987 for this one.


- Adam


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


On Nov. 2, 2017, 10:02 p.m., Adam Szita wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63528/
> ---
> 
> (Updated Nov. 2, 2017, 10:02 p.m.)
> 
> 
> Review request for hive, Peter Vary and Barna Zsombor Klara.
> 
> 
> Bugs: HIVE-17969
> https://issues.apache.org/jira/browse/HIVE-17969
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Refactoring alter table code to use batching of partitions when calling the 
> heavy removeUnusedColumnDescriptor method
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
>  ccadac1ada6aaae884ab39f5d99e91b8c542404e 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  62801c53853dbafb7c425cff943ec819dcee4800 
> 
> 
> Diff: https://reviews.apache.org/r/63528/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam Szita
> 
>

Re: Review Request 63528: HIVE-17969: Metastore to alter table in batches of partitions when renaming table

2017-11-06 Thread Adam Szita via Review Board



> On Nov. 2, 2017, 10:40 p.m., Peter Vary wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
> > Lines 265 (patched)
> > 
> >
> > Shouldn't we use 
> > parts = msdb.getPartitions(dbname, name, partitionBatchSize);
> > instead
> > parts = msdb.getPartitions(dbname, name, -1);
> > 
> > This way we will load only the required amount of partitions, instead 
> > of all.

Although msdb.getPartitions(dbname, name, partitionBatchSize) only loads 
partitionBatchSize amount of partitions, it will always return the same set of 
partitions. This is used in dropPartitions and it's not a problem there as 
we're also removing the requested set, so it is guaranteed to get new ones in 
the next call. For our use case here it is sadly not feasable to use this call. 
Taking a look at RawStore class I can see no iterative way to query the 
partitions from the DB behind HMS.


- Adam


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


On Nov. 2, 2017, 10:02 p.m., Adam Szita wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63528/
> ---
> 
> (Updated Nov. 2, 2017, 10:02 p.m.)
> 
> 
> Review request for hive, Peter Vary and Barna Zsombor Klara.
> 
> 
> Bugs: HIVE-17969
> https://issues.apache.org/jira/browse/HIVE-17969
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Refactoring alter table code to use batching of partitions when calling the 
> heavy removeUnusedColumnDescriptor method
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
>  ccadac1ada6aaae884ab39f5d99e91b8c542404e 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  62801c53853dbafb7c425cff943ec819dcee4800 
> 
> 
> Diff: https://reviews.apache.org/r/63528/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam Szita
> 
>

Review Request 63528: HIVE-17969: Metastore to alter table in batches of partitions when renaming table

2017-11-02 Thread Adam Szita via Review Board


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

Review request for hive, Peter Vary and Barna Zsombor Klara.


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


Repository: hive-git


Description
---

Refactoring alter table code to use batching of partitions when calling the 
heavy removeUnusedColumnDescriptor method


Diffs
-

  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
 ccadac1ada6aaae884ab39f5d99e91b8c542404e 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 62801c53853dbafb7c425cff943ec819dcee4800 


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


Testing
---


Thanks,

Adam Szita

Re: Review Request 63144: HIVE-16748: Integreate YETUS to Pre-Commit

2017-10-26 Thread Adam Szita via Review Board


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

(Updated Oct. 26, 2017, 7:04 p.m.)


Review request for hive, Peter Vary and Barna Zsombor Klara.


Changes
---

Addressed comments


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


Repository: hive-git


Description
---

We already have Yetus check script ready for run in dev-support. We should 
integrate this with the automated ptest infrastructure so that during Precommit 
test we get a Yetus result back as Jira comment


Diffs (updated)
-

  dev-support/hive-personality.sh f3247aac6284b8dd863691b4819a10c3a896d50c 
  testutils/ptest2/src/main/java/org/apache/hive/ptest/execution/PTest.java 
65a8216f6a076b0ee7baee11ca557f5e9f746316 
  
testutils/ptest2/src/main/java/org/apache/hive/ptest/execution/YetusPhase.java 
PRE-CREATION 
  testutils/ptest2/src/main/resources/source-prep.vm 
7ad50248af02dfaeb6524a61d4895f1a8efba211 
  testutils/ptest2/src/main/resources/yetus-exec.vm PRE-CREATION 


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

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


Testing
---

Tested on Cloudera sponsored sandbox ptest server.


Thanks,

Adam Szita

Review Request 63144: HIVE-16748: Integreate YETUS to Pre-Commit

2017-10-19 Thread Adam Szita via Review Board


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

Review request for hive, Peter Vary and Barna Zsombor Klara.


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


Repository: hive-git


Description
---

We already have Yetus check script ready for run in dev-support. We should 
integrate this with the automated ptest infrastructure so that during Precommit 
test we get a Yetus result back as Jira comment


Diffs
-

  dev-support/hive-personality.sh f3247aac6284b8dd863691b4819a10c3a896d50c 
  testutils/ptest2/src/main/java/org/apache/hive/ptest/execution/PTest.java 
65a8216f6a076b0ee7baee11ca557f5e9f746316 
  
testutils/ptest2/src/main/java/org/apache/hive/ptest/execution/YetusPhase.java 
PRE-CREATION 
  testutils/ptest2/src/main/resources/source-prep.vm 
7ad50248af02dfaeb6524a61d4895f1a8efba211 


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


Testing
---

Tested on Cloudera sponsored sandbox ptest server.


Thanks,

Adam Szita

53 matches

Mail list logo