HIVE-16213: ObjectStore can leak Queries when rollbackTransaction throws an exception (Vihang Karajgaonkar, reviewed by Sergio Pena)
Project: http://git-wip-us.apache.org/repos/asf/hive/repo Commit: http://git-wip-us.apache.org/repos/asf/hive/commit/c2b5dba7 Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/c2b5dba7 Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/c2b5dba7 Branch: refs/heads/branch-2.3 Commit: c2b5dba78e2ad1c9884cd4a54f7113532614b605 Parents: 145ed20 Author: Vihang Karajgaonkar <vih...@cloudera.com> Authored: Tue May 2 10:27:51 2017 -0500 Committer: Sahil Takiar <stak...@cloudera.com> Committed: Tue Nov 7 08:15:48 2017 -0800 ---------------------------------------------------------------------- .../hadoop/hive/metastore/ObjectStore.java | 549 ++++--------------- .../hadoop/hive/metastore/TestObjectStore.java | 14 + 2 files changed, 131 insertions(+), 432 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hive/blob/c2b5dba7/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java ---------------------------------------------------------------------- diff --git a/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java b/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java index 358cf17..e6a918b 100644 --- a/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java +++ b/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java @@ -234,26 +234,22 @@ public class ObjectStore implements RawStore, Configurable { private Pattern partitionValidationPattern; /** - * A class to pass the Query object to the caller to let the caller release - * resources by calling QueryWrapper.query.closeAll() after consuming all the query results. + * A Autocloseable wrapper around Query class to pass the Query object to the caller and let the caller release + * the resources when the QueryWrapper goes out of scope */ - public static class QueryWrapper { + public static class QueryWrapper implements AutoCloseable { public Query query; /** * Explicitly closes the query object to release the resources */ + @Override public void close() { if (query != null) { query.closeAll(); query = null; } } - - @Override - protected void finalize() { - this.close(); - } } public ObjectStore() { @@ -700,12 +696,7 @@ public class ObjectStore implements RawStore, Configurable { pm.retrieve(mdb); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } if (mdb == null) { throw new NoSuchObjectException("There is no database named " + name); @@ -824,10 +815,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return success; } @@ -858,12 +846,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return databases; } @@ -883,12 +866,7 @@ public class ObjectStore implements RawStore, Configurable { databases = new ArrayList<String>((Collection<String>) query.execute()); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } Collections.sort(databases); return databases; @@ -956,12 +934,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return type; } @@ -985,12 +958,7 @@ public class ObjectStore implements RawStore, Configurable { success = commitTransaction(); LOG.debug("type not found " + typeName, e); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return success; } @@ -1231,12 +1199,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return tbls; } @@ -1268,12 +1231,7 @@ public class ObjectStore implements RawStore, Configurable { result = (Long) query.execute(); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return result.intValue(); } @@ -1311,12 +1269,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return metas; } @@ -1402,12 +1355,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } nmtbl.mtbl = mtbl; return nmtbl; @@ -1450,15 +1398,10 @@ public class ObjectStore implements RawStore, Configurable { } committed = commitTransaction(); } finally { - if (!committed) { - rollbackTransaction(); - } + rollbackAndCleanup(committed, query); if (dbExistsQuery != null) { dbExistsQuery.closeAll(); } - if (query != null) { - query.closeAll(); - } } return tables; } @@ -1976,12 +1919,7 @@ public class ObjectStore implements RawStore, Configurable { } } } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return ret; } @@ -2213,10 +2151,7 @@ public class ObjectStore implements RawStore, Configurable { success = commitTransaction(); return parts; } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } } @@ -2318,6 +2253,7 @@ public class ObjectStore implements RawStore, Configurable { for (Iterator i = names.iterator(); i.hasNext();) { pns.add((String) i.next()); } + if (query != null) { query.closeAll(); } @@ -2412,10 +2348,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return partitions; } @@ -2437,10 +2370,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return partitionNames; } @@ -3206,12 +3136,7 @@ public class ObjectStore implements RawStore, Configurable { success = commitTransaction(); LOG.debug("Done retrieving all objects for listTableNamesByFilter"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return tableNames; } @@ -3257,12 +3182,7 @@ public class ObjectStore implements RawStore, Configurable { success = commitTransaction(); LOG.debug("Done retrieving all objects for listMPartitionNamesByFilter"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return partNames; } @@ -3481,10 +3401,7 @@ public class ObjectStore implements RawStore, Configurable { success = commitTransaction(); LOG.debug("successfully deleted a CD in removeUnusedColumnDescriptor"); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } } @@ -3568,12 +3485,7 @@ public class ObjectStore implements RawStore, Configurable { constraintNameIfExists = (String) constraintExistsQuery.execute(name); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (constraintExistsQuery != null) { - constraintExistsQuery.closeAll(); - } + rollbackAndCleanup(commited, constraintExistsQuery); } return constraintNameIfExists != null && !constraintNameIfExists.isEmpty(); } @@ -3821,12 +3733,7 @@ public class ObjectStore implements RawStore, Configurable { pm.retrieve(midx); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return midx; } @@ -3889,12 +3796,7 @@ public class ObjectStore implements RawStore, Configurable { return indexes; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -3921,12 +3823,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return pns; } @@ -4049,12 +3946,7 @@ public class ObjectStore implements RawStore, Configurable { pm.retrieve(mRoleMember); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return mRoleMember; } @@ -4123,11 +4015,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return success; } @@ -4197,12 +4085,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listRoles"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } if (principalType == PrincipalType.USER) { @@ -4268,7 +4151,6 @@ public class ObjectStore implements RawStore, Configurable { mRoleMemebership = (List<MRoleMap>) query.execute(roleName, principalType.toString()); pm.retrieveAll(mRoleMemebership); success = commitTransaction(); - LOG.debug("Done retrieving all objects for listMSecurityPrincipalMembershipRole"); } finally { if (!success) { @@ -4302,12 +4184,7 @@ public class ObjectStore implements RawStore, Configurable { pm.retrieve(mrole); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return mrole; } @@ -4329,12 +4206,7 @@ public class ObjectStore implements RawStore, Configurable { success = commitTransaction(); return roleNames; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -5160,12 +5032,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listRoleMembers"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mRoleMemeberList; } @@ -5216,12 +5083,7 @@ public class ObjectStore implements RawStore, Configurable { userNameDbPriv.addAll(mPrivs); } } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return userNameDbPriv; } @@ -5261,12 +5123,7 @@ public class ObjectStore implements RawStore, Configurable { commited = commitTransaction(); return convertGlobal(userNameDbPriv); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } } @@ -5309,12 +5166,7 @@ public class ObjectStore implements RawStore, Configurable { mSecurityDBList.addAll(mPrivs); LOG.debug("Done retrieving all objects for listPrincipalDBGrants"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mSecurityDBList; } @@ -5437,12 +5289,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listAllTableGrants"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mSecurityTabList; } @@ -5469,12 +5316,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listTableAllPartitionGrants"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mSecurityTabPartList; } @@ -5502,12 +5344,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listTableAllColumnGrants"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mTblColPrivilegeList; } @@ -5536,12 +5373,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listTableAllPartitionColumnGrants"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mSecurityColList; } @@ -5584,7 +5416,6 @@ public class ObjectStore implements RawStore, Configurable { private List<MDBPrivilege> listDatabaseGrants(String dbName, QueryWrapper queryWrapper) { dbName = HiveStringUtils.normalizeIdentifier(dbName); boolean success = false; - try { LOG.debug("Executing listDatabaseGrants"); @@ -5692,12 +5523,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listAllTableGrants"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mSecurityTabPartList; } @@ -5757,12 +5583,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalPartitionGrants"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mSecurityTabPartList; } @@ -5826,12 +5647,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalTableColumnGrants"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mSecurityColList; } @@ -5893,12 +5709,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalPartitionColumnGrants"); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return mSecurityColList; } @@ -5960,12 +5771,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalPartitionColumnGrantsAll"); return result; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -5993,12 +5799,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPartitionColumnGrantsAll"); return result; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -6073,12 +5874,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalAllTableGrants"); return result; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -6101,12 +5897,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalAllTableGrants"); return result; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -6146,7 +5937,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalAllPartitionGrants"); } finally { if (!success) { - rollbackTransaction(); + rollbackTransaction(); } } return mSecurityTabPartList; @@ -6178,12 +5969,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalPartitionGrantsAll"); return result; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -6209,12 +5995,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalPartitionGrantsAll"); return result; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -6292,12 +6073,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalTableColumnGrantsAll"); return result; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -6322,12 +6098,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalTableColumnGrantsAll"); return result; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -6404,12 +6175,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done executing isPartitionMarkedForEvent"); return (partEvents != null && !partEvents.isEmpty()) ? true : false; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -6463,7 +6229,6 @@ public class ObjectStore implements RawStore, Configurable { public Collection<?> executeJDOQLSelect(String queryStr, QueryWrapper queryWrapper) { boolean committed = false; Collection<?> result = null; - try { openTransaction(); Query query = queryWrapper.query = pm.newQuery(queryStr); @@ -6504,12 +6269,7 @@ public class ObjectStore implements RawStore, Configurable { return -1; } } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6539,12 +6299,7 @@ public class ObjectStore implements RawStore, Configurable { return null; } } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6655,12 +6410,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6748,12 +6498,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6787,12 +6532,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6883,12 +6623,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6965,12 +6700,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -7181,7 +6911,6 @@ public class ObjectStore implements RawStore, Configurable { } boolean committed = false; - try { openTransaction(); @@ -7506,12 +7235,7 @@ public class ObjectStore implements RawStore, Configurable { rollbackTransaction(); throw e; } finally { - if (!ret) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(ret, query); } return ret; } @@ -7581,12 +7305,7 @@ public class ObjectStore implements RawStore, Configurable { rollbackTransaction(); throw e; } finally { - if (!ret) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(ret, query); } return ret; } @@ -7608,12 +7327,7 @@ public class ObjectStore implements RawStore, Configurable { delCnt = query.deletePersistentAll(curTime, expiryTime); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); LOG.debug("Done executing cleanupEvents"); } return delCnt; @@ -7717,12 +7431,7 @@ public class ObjectStore implements RawStore, Configurable { return tokenIdents; } finally { LOG.debug("Done executing getAllTokenIdentifers with status : " + committed); - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -7765,12 +7474,7 @@ public class ObjectStore implements RawStore, Configurable { } committed = commitTransaction(); } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } LOG.debug("Done executing updateMasterKey with status : " + committed); if (null == masterKey) { @@ -7798,12 +7502,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } LOG.debug("Done executing removeMasterKey with status : " + success); return (null != masterKey) && success; @@ -7829,12 +7528,7 @@ public class ObjectStore implements RawStore, Configurable { return masterKeys; } finally { LOG.debug("Done executing getMasterKeys with status : " + committed); - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -7946,12 +7640,7 @@ public class ObjectStore implements RawStore, Configurable { } return mVerTables.get(0); } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -8177,12 +7866,7 @@ public class ObjectStore implements RawStore, Configurable { pm.retrieve(mfunc); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return mfunc; } @@ -8246,12 +7930,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return funcs; } @@ -8282,11 +7961,8 @@ public class ObjectStore implements RawStore, Configurable { } return result; } finally { - if (query != null) { - query.closeAll(); - } if (!commited) { - rollbackTransaction(); + rollbackAndCleanup(commited, query); return null; } } @@ -8316,12 +7992,7 @@ public class ObjectStore implements RawStore, Configurable { pm.makePersistent(translateThriftToDb(entry)); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } } @@ -8341,12 +8012,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } } @@ -8365,12 +8031,7 @@ public class ObjectStore implements RawStore, Configurable { commited = commitTransaction(); return new CurrentNotificationEventId(id); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } } @@ -8587,12 +8248,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return primaryKeys; } @@ -8617,12 +8273,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return ret; } @@ -8741,12 +8392,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return foreignKeys; } @@ -8774,4 +8420,43 @@ public class ObjectStore implements RawStore, Configurable { } } + /** + * This is a cleanup method which is used to rollback a active transaction + * if the success flag is false and close the associated Query object. This method is used + * internally and visible for testing purposes only + * @param success Rollback the current active transaction if false + * @param query Query object which needs to be closed + */ + @VisibleForTesting + void rollbackAndCleanup(boolean success, Query query) { + try { + if(!success) { + rollbackTransaction(); + } + } finally { + if (query != null) { + query.closeAll(); + } + } + } + + /** + * This is a cleanup method which is used to rollback a active transaction + * if the success flag is false and close the associated QueryWrapper object. This method is used + * internally and visible for testing purposes only + * @param success Rollback the current active transaction if false + * @param queryWrapper QueryWrapper object which needs to be closed + */ + @VisibleForTesting + void rollbackAndCleanup(boolean success, QueryWrapper queryWrapper) { + try { + if(!success) { + rollbackTransaction(); + } + } finally { + if (queryWrapper != null) { + queryWrapper.close(); + } + } + } } http://git-wip-us.apache.org/repos/asf/hive/blob/c2b5dba7/metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java ---------------------------------------------------------------------- diff --git a/metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java b/metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java index 6524ee7..69e8826 100644 --- a/metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java +++ b/metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java @@ -59,9 +59,12 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.mockito.Mockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.jdo.Query; + public class TestObjectStore { private ObjectStore objectStore = null; @@ -410,4 +413,15 @@ public class TestObjectStore { } catch (NoSuchObjectException e) { } } + + @Test + public void testQueryCloseOnError() throws Exception { + ObjectStore spy = Mockito.spy(objectStore); + spy.getAllDatabases(); + spy.getAllFunctions(); + spy.getAllTables(DB1); + spy.getPartitionCount(); + Mockito.verify(spy, Mockito.times(3)) + .rollbackAndCleanup(Mockito.anyBoolean(), Mockito.<Query>anyObject()); + } }