Re: Review Request 70256: HIVE-21480: Fixed flaky and broken test TestHiveMetaStore.testJDOPersistenceManagerCleanup

2019-03-22 Thread Peter Vary via Review Board 


> On márc. 21, 2019, 10:18 de, Peter Vary wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
> > Lines 3153-3155 (patched)
> > 
> >
> > My concern here is that we testing a different case with the patch.
> > 
> > Before we tested that we open/use/close a client and do not have 
> > lingering object. After the patch we test that closing the client will 
> > remove the 1 object - which says that the getAllDatabases will result in 
> > exactly 1 object.
> > 
> > How could that happen that there are lingering objects when we create a 
> > new client? Is there a way to get rid of the lingering object somehow, and 
> > then test the original usecase?
> 
> Morio Ramdenbourg wrote:
> My knowledge on the PersistenceManager code isn't that great, but before 
> the new client is even created, the object count returned from 
> getJDOPersistenceManagerCacheSize() is 1. I believe this object comes from 
> when the HMS is initializing the database schema. There seems to be a 
> lingering object from that, at least when I run this test as a standalone 
> without the other tests in the class.
> 
> Morio Ramdenbourg wrote:
> Do you know of other ways I can clear / wait for the PMF cache to empty 
> itself?

Me neither :(
Maybe 
https://docs.oracle.com/cd/E13189_01/kodo/docs303/jdo-javadoc/javax/jdo/PersistenceManager.html#evictAll()
 this? If not then after marking them to evict and some waiting?


- Peter


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


On márc. 20, 2019, 6:43 du, Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70256/
> ---
> 
> (Updated márc. 20, 2019, 6:43 du)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Karen Coppage, 
> Peter Vary, and Vihang Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This test was not correctly counting the number of
> objects in the PersistenceManager cache before and after 
> HiveMetaStoreClient.close(). The
> getJDOPersistenceManagerCacheSize() internal helper method did not use
> the updated fields present in the metastore classes, and was
> consistently returning -1. Additionally, there was a chance to cause
> flakiness since the object count before and after close() could
> differ depending on lingering objects from previous
> tests or setup.
> 
> Modified the helper method to use the new
> fields, and fixed the flakiness on this test.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
>  77e0c98265e7b561f2eb39536e3251dd92e9cab0 
> 
> 
> Diff: https://reviews.apache.org/r/70256/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests run
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 70256: HIVE-21480: Fixed flaky and broken test TestHiveMetaStore.testJDOPersistenceManagerCleanup

2019-03-21 Thread Morio Ramdenbourg via Review Board 


> On March 21, 2019, 10:18 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
> > Lines 3153-3155 (patched)
> > 
> >
> > My concern here is that we testing a different case with the patch.
> > 
> > Before we tested that we open/use/close a client and do not have 
> > lingering object. After the patch we test that closing the client will 
> > remove the 1 object - which says that the getAllDatabases will result in 
> > exactly 1 object.
> > 
> > How could that happen that there are lingering objects when we create a 
> > new client? Is there a way to get rid of the lingering object somehow, and 
> > then test the original usecase?
> 
> Morio Ramdenbourg wrote:
> My knowledge on the PersistenceManager code isn't that great, but before 
> the new client is even created, the object count returned from 
> getJDOPersistenceManagerCacheSize() is 1. I believe this object comes from 
> when the HMS is initializing the database schema. There seems to be a 
> lingering object from that, at least when I run this test as a standalone 
> without the other tests in the class.

Do you know of other ways I can clear / wait for the PMF cache to empty itself?


- Morio


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


On March 20, 2019, 6:43 p.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70256/
> ---
> 
> (Updated March 20, 2019, 6:43 p.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Karen Coppage, 
> Peter Vary, and Vihang Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This test was not correctly counting the number of
> objects in the PersistenceManager cache before and after 
> HiveMetaStoreClient.close(). The
> getJDOPersistenceManagerCacheSize() internal helper method did not use
> the updated fields present in the metastore classes, and was
> consistently returning -1. Additionally, there was a chance to cause
> flakiness since the object count before and after close() could
> differ depending on lingering objects from previous
> tests or setup.
> 
> Modified the helper method to use the new
> fields, and fixed the flakiness on this test.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
>  77e0c98265e7b561f2eb39536e3251dd92e9cab0 
> 
> 
> Diff: https://reviews.apache.org/r/70256/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests run
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 70256: HIVE-21480: Fixed flaky and broken test TestHiveMetaStore.testJDOPersistenceManagerCleanup

2019-03-21 Thread Morio Ramdenbourg via Review Board 


> On March 21, 2019, 10:18 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
> > Lines 3153-3155 (patched)
> > 
> >
> > My concern here is that we testing a different case with the patch.
> > 
> > Before we tested that we open/use/close a client and do not have 
> > lingering object. After the patch we test that closing the client will 
> > remove the 1 object - which says that the getAllDatabases will result in 
> > exactly 1 object.
> > 
> > How could that happen that there are lingering objects when we create a 
> > new client? Is there a way to get rid of the lingering object somehow, and 
> > then test the original usecase?

My knowledge on the PersistenceManager code isn't that great, but before the 
new client is even created, the object count returned from 
getJDOPersistenceManagerCacheSize() is 1. I believe this object comes from when 
the HMS is initializing the database schema. There seems to be a lingering 
object from that, at least when I run this test as a standalone without the 
other tests in the class.


- Morio


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


On March 20, 2019, 6:43 p.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70256/
> ---
> 
> (Updated March 20, 2019, 6:43 p.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Karen Coppage, 
> Peter Vary, and Vihang Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This test was not correctly counting the number of
> objects in the PersistenceManager cache before and after 
> HiveMetaStoreClient.close(). The
> getJDOPersistenceManagerCacheSize() internal helper method did not use
> the updated fields present in the metastore classes, and was
> consistently returning -1. Additionally, there was a chance to cause
> flakiness since the object count before and after close() could
> differ depending on lingering objects from previous
> tests or setup.
> 
> Modified the helper method to use the new
> fields, and fixed the flakiness on this test.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
>  77e0c98265e7b561f2eb39536e3251dd92e9cab0 
> 
> 
> Diff: https://reviews.apache.org/r/70256/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests run
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 70256: HIVE-21480: Fixed flaky and broken test TestHiveMetaStore.testJDOPersistenceManagerCleanup

2019-03-21 Thread Peter Vary via Review Board 

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




standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
Lines 3153-3155 (patched)


My concern here is that we testing a different case with the patch.

Before we tested that we open/use/close a client and do not have lingering 
object. After the patch we test that closing the client will remove the 1 
object - which says that the getAllDatabases will result in exactly 1 object.

How could that happen that there are lingering objects when we create a new 
client? Is there a way to get rid of the lingering object somehow, and then 
test the original usecase?


- Peter Vary


On márc. 20, 2019, 6:43 du, Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70256/
> ---
> 
> (Updated márc. 20, 2019, 6:43 du)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Karen Coppage, 
> Peter Vary, and Vihang Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This test was not correctly counting the number of
> objects in the PersistenceManager cache before and after 
> HiveMetaStoreClient.close(). The
> getJDOPersistenceManagerCacheSize() internal helper method did not use
> the updated fields present in the metastore classes, and was
> consistently returning -1. Additionally, there was a chance to cause
> flakiness since the object count before and after close() could
> differ depending on lingering objects from previous
> tests or setup.
> 
> Modified the helper method to use the new
> fields, and fixed the flakiness on this test.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
>  77e0c98265e7b561f2eb39536e3251dd92e9cab0 
> 
> 
> Diff: https://reviews.apache.org/r/70256/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests run
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 70256: HIVE-21480: Fixed flaky and broken test TestHiveMetaStore.testJDOPersistenceManagerCleanup

2019-03-20 Thread Morio Ramdenbourg via Review Board 


> On March 20, 2019, 6:41 p.m., Karthik Manamcheri wrote:
> > LGTM. get someone else a little more familiar with the tests (vihang, pvary 
> > or someone) to also do a quick review. thanks.

Thanks for the review :)


- Morio


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


On March 20, 2019, 6:43 p.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70256/
> ---
> 
> (Updated March 20, 2019, 6:43 p.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Karen Coppage, 
> Peter Vary, and Vihang Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This test was not correctly counting the number of
> objects in the PersistenceManager cache before and after 
> HiveMetaStoreClient.close(). The
> getJDOPersistenceManagerCacheSize() internal helper method did not use
> the updated fields present in the metastore classes, and was
> consistently returning -1. Additionally, there was a chance to cause
> flakiness since the object count before and after close() could
> differ depending on lingering objects from previous
> tests or setup.
> 
> Modified the helper method to use the new
> fields, and fixed the flakiness on this test.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
>  77e0c98265e7b561f2eb39536e3251dd92e9cab0 
> 
> 
> Diff: https://reviews.apache.org/r/70256/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests run
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 70256: HIVE-21480: Fixed flaky and broken test TestHiveMetaStore.testJDOPersistenceManagerCleanup

2019-03-20 Thread Karthik Manamcheri via Review Board 

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


Ship it!




LGTM. get someone else a little more familiar with the tests (vihang, pvary or 
someone) to also do a quick review. thanks.

- Karthik Manamcheri


On March 20, 2019, 5:07 p.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70256/
> ---
> 
> (Updated March 20, 2019, 5:07 p.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Karen Coppage, and 
> Vihang Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This test was not correctly counting the number of
> objects in the PersistenceManager cache before and after 
> HiveMetaStoreClient.close(). The
> getJDOPersistenceManagerCacheSize() internal helper method did not use
> the updated fields present in the metastore classes, and was
> consistently returning -1. Additionally, there was a chance to cause
> flakiness since the object count before and after close() could
> differ depending on lingering objects from previous
> tests or setup.
> 
> Modified the helper method to use the new
> fields, and fixed the flakiness on this test.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
>  77e0c98265e7b561f2eb39536e3251dd92e9cab0 
> 
> 
> Diff: https://reviews.apache.org/r/70256/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests run
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Review Request 70256: HIVE-21480: Fixed flaky and broken test TestHiveMetaStore.testJDOPersistenceManagerCleanup

2019-03-20 Thread Morio Ramdenbourg via Review Board 

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

Review request for hive, Adam Holley, Karthik Manamcheri, Karen Coppage, and 
Vihang Karajgaonkar.


Repository: hive-git


Description
---

This test was not correctly counting the number of
objects in the PersistenceManager cache before and after 
HiveMetaStoreClient.close(). The
getJDOPersistenceManagerCacheSize() internal helper method did not use
the updated fields present in the metastore classes, and was
consistently returning -1. Additionally, there was a chance to cause
flakiness since the object count before and after close() could
differ depending on lingering objects from previous
tests or setup.

Modified the helper method to use the new
fields, and fixed the flakiness on this test.


Diffs
-

  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
 77e0c98265e7b561f2eb39536e3251dd92e9cab0 


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


Testing
---

Unit tests run


Thanks,

Morio Ramdenbourg