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

2018-11-29 Thread Naveen Gangam via Review Board 

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


Ship it!




Ship It!

- Naveen Gangam


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

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

2018-11-27 Thread Vihang Karajgaonkar via Review Board 


> On Nov. 27, 2018, 10:55 p.m., Naveen Gangam wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java
> > Lines 162 (patched)
> > 
> >
> > Should we acquire the writeLock before releasing the readLock to 
> > prevent another thread from entering this code? This way we dont have to 
> > re-check these as well
> >   if (prop == null || pmf == null || !propsFromConf.equals(prop))

AFAIK we cannot upgrade from readLock to a writeLock when using 
ReentrantReadWriteLock, only downgrade is possible (a thread which holds write 
lock can acquire read lock) 
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html


- Vihang


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


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

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

2018-11-27 Thread Naveen Gangam via Review Board 

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




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


Should we acquire the writeLock before releasing the readLock to prevent 
another thread from entering this code? This way we dont have to re-check these 
as well
  if (prop == null || pmf == null || !propsFromConf.equals(prop))


- Naveen Gangam


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

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

2018-11-27 Thread Andrew Sherman via Review Board 

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



LGTM


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


nit: delete



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


nit: delete


- Andrew Sherman


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

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

2018-11-26 Thread Vihang Karajgaonkar via Review Board 

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

(Updated Nov. 27, 2018, 7:18 a.m.)


Review request for hive, Andrew Sherman, Alan Gates, and Peter Vary.


Changes
---

Rebased to the latest code on master.


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


Repository: hive-git


Description
---

HIVE-20740 : Remove global lock in ObjectStore.setConf method


Diffs (updated)
-

  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenarios.java
 5a88550f0625a7ec1890df7f54e7fa579f58fff4 
  itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
5cb0a887e672f49739f5b648e608fba66de06326 
  ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
455ffc3887e62fa503cc3fa28255702ea9da3cc0 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 570281b54fa236d5bb568b4ded9b166ef367f613 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java
 PRE-CREATION 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 af9efd98ea210335c6ac1d3da8624e02aadc2706 


Diff: https://reviews.apache.org/r/69054/diff/6/

Changes: https://reviews.apache.org/r/69054/diff/5-6/


Testing
---


Thanks,

Vihang Karajgaonkar

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

2018-11-01 Thread Vihang Karajgaonkar via Review Board 


> On Oct. 18, 2018, 2:33 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Line 244 (original), 341 (patched)
> > 
> >
> > You are ignoring the return value? Should you have 
> > pmf=getUpdatedPmfIfNeeded(..)?
> 
> Vihang Karajgaonkar wrote:
> The pmf is updated by the method if needed, so we don't need to use the 
> return value. Will rename the method to updatePmfIfNeeded to make it more 
> readable.
> 
> Karthik Manamcheri wrote:
> And you can have it has void return.

comment does not apply to latest version of the patch. Dropping it.


> On Oct. 18, 2018, 2:33 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Line 287 (original), 374 (patched)
> > 
> >
> > What is dss.log?

Actually, I don't know what it is. Looking at the git history, this line has 
been there forever. Looks like its about time to update it :)


- Vihang


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


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

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

2018-11-01 Thread Vihang Karajgaonkar via Review Board 

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

(Updated Nov. 1, 2018, 8:48 p.m.)


Review request for hive, Andrew Sherman, Alan Gates, and Peter Vary.


Changes
---

Fixed failing tests


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


Repository: hive-git


Description
---

HIVE-20740 : Remove global lock in ObjectStore.setConf method


Diffs (updated)
-

  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenarios.java
 75cd68a9d6be6c5804e458d19a0023f0d7f5beae 
  itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniHS2.java 
5cb0a887e672f49739f5b648e608fba66de06326 
  ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
ee7c940d2b7fbd66af2d006da0585c6b42b9b0bb 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 03e3a2d2573b54651833867b906821650f4fb9c1 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java
 PRE-CREATION 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 b74c3048fa2e18adc7f0d7cc813a180d4466fa36 


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

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


Testing
---


Thanks,

Vihang Karajgaonkar

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

2018-10-22 Thread Andrew Sherman via Review Board 


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

OK with me


- Andrew


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


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

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

2018-10-19 Thread Vihang Karajgaonkar via Review Board 

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

(Updated Oct. 19, 2018, 8:54 p.m.)


Review request for hive, Andrew Sherman, Alan Gates, and Peter Vary.


Changes
---

Fixed couple of bugs in the patch to fix the test failures.


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


Repository: hive-git


Description
---

HIVE-20740 : Remove global lock in ObjectStore.setConf method


Diffs (updated)
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 66977d79c946f1ac57aacfbe8704d37bfbac3ea3 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 b74c3048fa2e18adc7f0d7cc813a180d4466fa36 


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

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


Testing
---


Thanks,

Vihang Karajgaonkar

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

2018-10-19 Thread Karthik Manamcheri via Review Board 


> On Oct. 18, 2018, 2:33 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Line 244 (original), 341 (patched)
> > 
> >
> > You are ignoring the return value? Should you have 
> > pmf=getUpdatedPmfIfNeeded(..)?
> 
> Vihang Karajgaonkar wrote:
> The pmf is updated by the method if needed, so we don't need to use the 
> return value. Will rename the method to updatePmfIfNeeded to make it more 
> readable.

And you can have it has void return.


> On Oct. 18, 2018, 2:33 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
> > Lines 927 (patched)
> > 
> >
> > Is this testing the new read/write lock behavior? As in, would this 
> > test have failed if you had the previous behavior of a global lock?
> 
> Vihang Karajgaonkar wrote:
> This is performance fix. There was nothing broken in the previous 
> implementation so its hard to come up with a test which break on the previous 
> code. The purpose of this test to cover the newly added code so that if there 
> are any obvious bugs they will be seen. If you have more ideas on how can we 
> add more tests please suggest and I would be happy to add them.

Ok sounds fair enough.

We could add performance regression tests, like for example, spin up 100 
threads all creating ObjectStores in parallel. With your code, they all should 
pretty much be instantaneous (since they never lock on each other unless config 
changes). If we accidentally regress, the test could catch it. What do you 
think? We don't have to do this as part of this ticket. We can do it as a 
separate perf regression test.


- Karthik


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


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

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

2018-10-18 Thread Vihang Karajgaonkar via Review Board 


> On Oct. 18, 2018, 2:33 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Line 244 (original), 341 (patched)
> > 
> >
> > You are ignoring the return value? Should you have 
> > pmf=getUpdatedPmfIfNeeded(..)?

The pmf is updated by the method if needed, so we don't need to use the return 
value. Will rename the method to updatePmfIfNeeded to make it more readable.


> On Oct. 18, 2018, 2:33 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
> > Lines 927 (patched)
> > 
> >
> > Is this testing the new read/write lock behavior? As in, would this 
> > test have failed if you had the previous behavior of a global lock?

This is performance fix. There was nothing broken in the previous 
implementation so its hard to come up with a test which break on the previous 
code. The purpose of this test to cover the newly added code so that if there 
are any obvious bugs they will be seen. If you have more ideas on how can we 
add more tests please suggest and I would be happy to add them.


- Vihang


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


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

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

2018-10-18 Thread Karthik Manamcheri via Review Board 

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




standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Line 244 (original), 341 (patched)


You are ignoring the return value? Should you have 
pmf=getUpdatedPmfIfNeeded(..)?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Line 287 (original), 374 (patched)


What is dss.log?



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


Is this testing the new read/write lock behavior? As in, would this test 
have failed if you had the previous behavior of a global lock?


- Karthik Manamcheri


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

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

2018-10-16 Thread Vihang Karajgaonkar via Review Board 


> On Oct. 16, 2018, 8:59 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 291 (patched)
> > 
> >
> > What is someone has set  ConfVars.MANAGER_FACTORY_CLASS to some 
> > non-default value? Is this still correct?

Yes, looks like it will fail in that case although I am not sure the use-cases 
where you will use a different PersistenceManagerFactory. This code has been 
there since before the patch and has not been changed in this patch. Perhaps we 
can look at this as a separate JIRA.


- Vihang


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


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

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

2018-10-16 Thread Vihang Karajgaonkar via Review Board 

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

(Updated Oct. 16, 2018, 10:47 p.m.)


Review request for hive, Andrew Sherman, Alan Gates, and Peter Vary.


Changes
---

Added suggested changes from Andrew


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


Repository: hive-git


Description
---

HIVE-20740 : Remove global lock in ObjectStore.setConf method


Diffs (updated)
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 66977d79c946f1ac57aacfbe8704d37bfbac3ea3 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 b74c3048fa2e18adc7f0d7cc813a180d4466fa36 


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

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


Testing
---


Thanks,

Vihang Karajgaonkar

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

2018-10-16 Thread Andrew Sherman via Review Board 

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



This all looks good, I just have annoying questions


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


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



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


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



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


Add comment expaining what the test does



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


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



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


ArrayList<>(numThreads) ?



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


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


- Andrew Sherman


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

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

2018-10-16 Thread Vihang Karajgaonkar via Review Board 

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

Review request for hive, Andrew Sherman, Alan Gates, and Peter Vary.


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


Repository: hive-git


Description
---

HIVE-20740 : Remove global lock in ObjectStore.setConf method


Diffs
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 66977d79c946f1ac57aacfbe8704d37bfbac3ea3 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 b74c3048fa2e18adc7f0d7cc813a180d4466fa36 


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


Testing
---


Thanks,

Vihang Karajgaonkar