You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Aihua Xu <ax...@cloudera.com> on 2015/06/23 23:26:23 UTC
Review Request 35803: HIVE-10895 ObjectStore does not close Query
objects in
some calls, causing a potential leak in some metastore db resources
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35803/
-----------------------------------------------------------
Review request for hive.
Repository: hive-git
Description
-------
HIVE-10895 ObjectStore does not close Query objects in some calls, causing a potential leak in some metastore db resources
Diffs
-----
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 417ecc825e4bca8c92f3e42c19e693b624c96dc1
metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java PRE-CREATION
Diff: https://reviews.apache.org/r/35803/diff/
Testing
-------
Thanks,
Aihua Xu
Re: Review Request 35803: HIVE-10895 ObjectStore does not close Query
objects in some calls,
causing a potential leak in some metastore db resources
Posted by Vaibhav Gumashta <vg...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35803/#review90459
-----------------------------------------------------------
Ship it!
Ship It!
- Vaibhav Gumashta
On June 30, 2015, 1:39 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35803/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:39 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-10895 ObjectStore does not close Query objects in some calls, causing a potential leak in some metastore db resources
>
>
> Diffs
> -----
>
> metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 1c21c8b
> metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 417ecc8
> metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java d0ff329
> metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35803/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 35803: HIVE-10895 ObjectStore does not close Query
objects in some calls,
causing a potential leak in some metastore db resources
Posted by Aihua Xu <ax...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35803/
-----------------------------------------------------------
(Updated June 30, 2015, 1:39 p.m.)
Review request for hive.
Repository: hive-git
Description
-------
HIVE-10895 ObjectStore does not close Query objects in some calls, causing a potential leak in some metastore db resources
Diffs (updated)
-----
metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 1c21c8b
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 417ecc8
metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java d0ff329
metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java PRE-CREATION
Diff: https://reviews.apache.org/r/35803/diff/
Testing (updated)
-------
Thanks,
Aihua Xu
Re: Review Request 35803: HIVE-10895 ObjectStore does not close Query
objects in some calls,
causing a potential leak in some metastore db resources
Posted by Chaoyu Tang <ct...@gmail.com>.
> On June 29, 2015, 4:55 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line 3477
> > <https://reviews.apache.org/r/35803/diff/3/?file=993090#file993090line3477>
> >
> > Do we have to use a list of queryWrappers with each wrapper for a different listXXX call? If so, we need open eight queries for this removeRole methods. Could we use only one and reuse it?
>
> Aihua Xu wrote:
> Originally we were already using eight queries and I was not trying to change that logic in this patch. And also we can't reuse the same query since we need to release the resource for each of the query anyway.
What I meant reuse is that to use only one queryWrapper and close it after each call to listXXX, so there will be at most one open query (cursor) at a time in this method and help the scalability.
> On June 29, 2015, 4:55 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line 3485
> > <https://reviews.apache.org/r/35803/diff/3/?file=993090#file993090line3485>
> >
> > Should listPrincipalGlobalGrants be passed in a queryWrapper as well?
>
> Aihua Xu wrote:
> listPrincipalGlobalGrants has handled the releasing the resource by itself, so we don't need to pass that in.
In local copy of code, the query in this method is not self closed (line 4395)
{code}
@SuppressWarnings("unchecked")
@Override
public List<MGlobalPrivilege> listPrincipalGlobalGrants(String principalName, PrincipalType principalType) {
boolean commited = false;
List<MGlobalPrivilege> userNameDbPriv = null;
try {
openTransaction();
if (principalName != null) {
Query query = pm.newQuery(MGlobalPrivilege.class,
"principalName == t1 && principalType == t2 ");
query.declareParameters(
"java.lang.String t1, java.lang.String t2");
userNameDbPriv = (List<MGlobalPrivilege>) query
.executeWithArray(principalName, principalType.toString());
pm.retrieveAll(userNameDbPriv);
}
commited = commitTransaction();
} finally {
if (!commited) {
rollbackTransaction();
}
}
return userNameDbPriv;
}
{code}
- Chaoyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35803/#review89749
-----------------------------------------------------------
On June 26, 2015, 5:23 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35803/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 5:23 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-10895 ObjectStore does not close Query objects in some calls, causing a potential leak in some metastore db resources
>
>
> Diffs
> -----
>
> metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 417ecc8
> metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java d0ff329
> metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35803/diff/
>
>
> Testing
> -------
>
> Testing has been done.
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 35803: HIVE-10895 ObjectStore does not close Query
objects in some calls,
causing a potential leak in some metastore db resources
Posted by Aihua Xu <ax...@cloudera.com>.
> On June 29, 2015, 4:55 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line 3485
> > <https://reviews.apache.org/r/35803/diff/3/?file=993090#file993090line3485>
> >
> > Should listPrincipalGlobalGrants be passed in a queryWrapper as well?
>
> Aihua Xu wrote:
> listPrincipalGlobalGrants has handled the releasing the resource by itself, so we don't need to pass that in.
>
> Chaoyu Tang wrote:
> In local copy of code, the query in this method is not self closed (line 4395)
> {code}
> @SuppressWarnings("unchecked")
> @Override
> public List<MGlobalPrivilege> listPrincipalGlobalGrants(String principalName, PrincipalType principalType) {
> boolean commited = false;
> List<MGlobalPrivilege> userNameDbPriv = null;
> try {
> openTransaction();
> if (principalName != null) {
> Query query = pm.newQuery(MGlobalPrivilege.class,
> "principalName == t1 && principalType == t2 ");
> query.declareParameters(
> "java.lang.String t1, java.lang.String t2");
> userNameDbPriv = (List<MGlobalPrivilege>) query
> .executeWithArray(principalName, principalType.toString());
> pm.retrieveAll(userNameDbPriv);
> }
> commited = commitTransaction();
> } finally {
> if (!commited) {
> rollbackTransaction();
> }
> }
> return userNameDbPriv;
> }
> {code}
My patch will close the query. See line 4493.
> On June 29, 2015, 4:55 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line 3477
> > <https://reviews.apache.org/r/35803/diff/3/?file=993090#file993090line3477>
> >
> > Do we have to use a list of queryWrappers with each wrapper for a different listXXX call? If so, we need open eight queries for this removeRole methods. Could we use only one and reuse it?
>
> Aihua Xu wrote:
> Originally we were already using eight queries and I was not trying to change that logic in this patch. And also we can't reuse the same query since we need to release the resource for each of the query anyway.
>
> Chaoyu Tang wrote:
> What I meant reuse is that to use only one queryWrapper and close it after each call to listXXX, so there will be at most one open query (cursor) at a time in this method and help the scalability.
Yeah. We can do that.
- Aihua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35803/#review89749
-----------------------------------------------------------
On June 26, 2015, 5:23 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35803/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 5:23 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-10895 ObjectStore does not close Query objects in some calls, causing a potential leak in some metastore db resources
>
>
> Diffs
> -----
>
> metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 417ecc8
> metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java d0ff329
> metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35803/diff/
>
>
> Testing
> -------
>
> Testing has been done.
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 35803: HIVE-10895 ObjectStore does not close Query
objects in some calls,
causing a potential leak in some metastore db resources
Posted by Aihua Xu <ax...@cloudera.com>.
> On June 29, 2015, 4:55 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line 3477
> > <https://reviews.apache.org/r/35803/diff/3/?file=993090#file993090line3477>
> >
> > Do we have to use a list of queryWrappers with each wrapper for a different listXXX call? If so, we need open eight queries for this removeRole methods. Could we use only one and reuse it?
Originally we were already using eight queries and I was not trying to change that logic in this patch. And also we can't reuse the same query since we need to release the resource for each of the query anyway.
> On June 29, 2015, 4:55 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line 3485
> > <https://reviews.apache.org/r/35803/diff/3/?file=993090#file993090line3485>
> >
> > Should listPrincipalGlobalGrants be passed in a queryWrapper as well?
listPrincipalGlobalGrants has handled the releasing the resource by itself, so we don't need to pass that in.
> On June 29, 2015, 4:55 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line 4939
> > <https://reviews.apache.org/r/35803/diff/3/?file=993090#file993090line4939>
> >
> > Is there any reason you need an additional list to hold the retrieved result?
Yeah. That is the place to get all the returned data so that we can close the query. Since it's the public interface, I didn't pass QueryWapper to let the caller to close it. If we think there is a memory issue for some calls, we can do that.
> On June 29, 2015, 4:55 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line 1793
> > <https://reviews.apache.org/r/35803/diff/3/?file=993090#file993090line1793>
> >
> > Have you looked at MetaStoreDirectSql class to see if there is any potential query leaking?
I will take a look at that class.
- Aihua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35803/#review89749
-----------------------------------------------------------
On June 26, 2015, 5:23 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35803/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 5:23 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-10895 ObjectStore does not close Query objects in some calls, causing a potential leak in some metastore db resources
>
>
> Diffs
> -----
>
> metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 417ecc8
> metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java d0ff329
> metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35803/diff/
>
>
> Testing
> -------
>
> Testing has been done.
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 35803: HIVE-10895 ObjectStore does not close Query
objects in some calls,
causing a potential leak in some metastore db resources
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35803/#review89749
-----------------------------------------------------------
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 1059)
<https://reviews.apache.org/r/35803/#comment142533>
Indentation ...
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 1089)
<https://reviews.apache.org/r/35803/#comment142535>
Indentation..
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 1788)
<https://reviews.apache.org/r/35803/#comment142541>
Have you looked at MetaStoreDirectSql class to see if there is any potential query leaking?
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 3409)
<https://reviews.apache.org/r/35803/#comment142539>
Do we have to use a list of queryWrappers with each wrapper for a different listXXX call? If so, we need open eight queries for this removeRole methods. Could we use only one and reuse it?
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 3417)
<https://reviews.apache.org/r/35803/#comment142538>
Should listPrincipalGlobalGrants be passed in a queryWrapper as well?
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 4765)
<https://reviews.apache.org/r/35803/#comment142540>
Is there any reason you need an additional list to hold the retrieved result?
- Chaoyu Tang
On June 26, 2015, 5:23 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35803/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 5:23 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-10895 ObjectStore does not close Query objects in some calls, causing a potential leak in some metastore db resources
>
>
> Diffs
> -----
>
> metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 417ecc8
> metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java d0ff329
> metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35803/diff/
>
>
> Testing
> -------
>
> Testing has been done.
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 35803: HIVE-10895 ObjectStore does not close Query
objects in some calls,
causing a potential leak in some metastore db resources
Posted by Aihua Xu <ax...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35803/
-----------------------------------------------------------
(Updated June 26, 2015, 5:23 p.m.)
Review request for hive.
Repository: hive-git
Description
-------
HIVE-10895 ObjectStore does not close Query objects in some calls, causing a potential leak in some metastore db resources
Diffs (updated)
-----
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 417ecc8
metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java d0ff329
metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java PRE-CREATION
Diff: https://reviews.apache.org/r/35803/diff/
Testing
-------
Testing has been done.
Thanks,
Aihua Xu
Re: Review Request 35803: HIVE-10895 ObjectStore does not close Query
objects in some calls,
causing a potential leak in some metastore db resources
Posted by Aihua Xu <ax...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35803/
-----------------------------------------------------------
(Updated June 24, 2015, 3:13 p.m.)
Review request for hive.
Repository: hive-git
Description
-------
HIVE-10895 ObjectStore does not close Query objects in some calls, causing a potential leak in some metastore db resources
Diffs (updated)
-----
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 417ecc8
metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java PRE-CREATION
Diff: https://reviews.apache.org/r/35803/diff/
Testing (updated)
-------
Testing has been done.
Thanks,
Aihua Xu