Repository: hive Updated Branches: refs/heads/master 53b70bdc3 -> 812fa3946
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/812fa394 Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/812fa394 Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/812fa394 Branch: refs/heads/master Commit: 812fa39463a49a6391b5f82c6bfbac589573c500 Parents: 53b70bd Author: Vihang Karajgaonkar <vih...@cloudera.com> Authored: Tue May 2 10:27:51 2017 -0500 Committer: Sergio Pena <sergio.p...@cloudera.com> Committed: Tue May 2 10:29:04 2017 -0500 ---------------------------------------------------------------------- .../hadoop/hive/metastore/ObjectStore.java | 550 ++++--------------- .../hadoop/hive/metastore/TestObjectStore.java | 14 + 2 files changed, 132 insertions(+), 432 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hive/blob/812fa394/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 58b7730..29aa642 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; } @@ -1234,12 +1202,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return tbls; } @@ -1271,12 +1234,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(); } @@ -1314,12 +1272,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return metas; } @@ -1405,12 +1358,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; @@ -1453,15 +1401,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; } @@ -1979,12 +1922,7 @@ public class ObjectStore implements RawStore, Configurable { } } } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return ret; } @@ -2216,10 +2154,7 @@ public class ObjectStore implements RawStore, Configurable { success = commitTransaction(); return parts; } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } } @@ -2321,6 +2256,7 @@ public class ObjectStore implements RawStore, Configurable { for (Iterator i = names.iterator(); i.hasNext();) { pns.add((String) i.next()); } + if (query != null) { query.closeAll(); } @@ -2415,10 +2351,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return partitions; } @@ -2440,10 +2373,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return partitionNames; } @@ -3209,12 +3139,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; } @@ -3260,12 +3185,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; } @@ -3484,10 +3404,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); } } @@ -3571,12 +3488,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(); } @@ -3824,12 +3736,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; } @@ -3892,12 +3799,7 @@ public class ObjectStore implements RawStore, Configurable { return indexes; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -3924,12 +3826,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return pns; } @@ -4052,12 +3949,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; } @@ -4126,11 +4018,7 @@ public class ObjectStore implements RawStore, Configurable { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return success; } @@ -4200,12 +4088,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) { @@ -4271,7 +4154,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) { @@ -4305,12 +4187,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; } @@ -4332,12 +4209,7 @@ public class ObjectStore implements RawStore, Configurable { success = commitTransaction(); return roleNames; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -5163,12 +5035,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; } @@ -5219,12 +5086,7 @@ public class ObjectStore implements RawStore, Configurable { userNameDbPriv.addAll(mPrivs); } } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return userNameDbPriv; } @@ -5264,12 +5126,7 @@ public class ObjectStore implements RawStore, Configurable { commited = commitTransaction(); return convertGlobal(userNameDbPriv); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } } @@ -5312,12 +5169,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; } @@ -5440,12 +5292,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; } @@ -5472,12 +5319,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; } @@ -5505,12 +5347,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; } @@ -5539,12 +5376,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; } @@ -5587,7 +5419,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"); @@ -5695,12 +5526,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; } @@ -5760,12 +5586,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; } @@ -5829,12 +5650,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; } @@ -5896,12 +5712,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; } @@ -5963,12 +5774,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); } } @@ -5996,12 +5802,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); } } @@ -6076,12 +5877,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); } } @@ -6104,12 +5900,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); } } @@ -6149,7 +5940,7 @@ public class ObjectStore implements RawStore, Configurable { LOG.debug("Done retrieving all objects for listPrincipalAllPartitionGrants"); } finally { if (!success) { - rollbackTransaction(); + rollbackTransaction(); } } return mSecurityTabPartList; @@ -6181,12 +5972,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); } } @@ -6212,12 +5998,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); } } @@ -6295,12 +6076,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); } } @@ -6325,12 +6101,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); } } @@ -6407,12 +6178,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); } } @@ -6466,7 +6232,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); @@ -6507,12 +6272,7 @@ public class ObjectStore implements RawStore, Configurable { return -1; } } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6542,12 +6302,7 @@ public class ObjectStore implements RawStore, Configurable { return null; } } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6658,12 +6413,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6751,12 +6501,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6790,12 +6535,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6886,12 +6626,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6968,12 +6703,7 @@ public class ObjectStore implements RawStore, Configurable { } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -7184,7 +6914,6 @@ public class ObjectStore implements RawStore, Configurable { } boolean committed = false; - try { openTransaction(); @@ -7546,12 +7275,7 @@ public class ObjectStore implements RawStore, Configurable { rollbackTransaction(); throw e; } finally { - if (!ret) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(ret, query); } return ret; } @@ -7621,12 +7345,7 @@ public class ObjectStore implements RawStore, Configurable { rollbackTransaction(); throw e; } finally { - if (!ret) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(ret, query); } return ret; } @@ -7648,12 +7367,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; @@ -7757,12 +7471,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); } } @@ -7805,12 +7514,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) { @@ -7838,12 +7542,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; @@ -7869,12 +7568,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); } } @@ -7986,12 +7680,7 @@ public class ObjectStore implements RawStore, Configurable { } return mVerTables.get(0); } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -8217,12 +7906,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; } @@ -8286,12 +7970,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return funcs; } @@ -8322,11 +8001,8 @@ public class ObjectStore implements RawStore, Configurable { } return result; } finally { - if (query != null) { - query.closeAll(); - } if (!commited) { - rollbackTransaction(); + rollbackAndCleanup(commited, query); return null; } } @@ -8356,12 +8032,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); } } @@ -8381,12 +8052,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } } @@ -8405,12 +8071,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); } } @@ -8629,12 +8290,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return primaryKeys; } @@ -8659,12 +8315,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return ret; } @@ -8787,12 +8438,7 @@ public class ObjectStore implements RawStore, Configurable { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return foreignKeys; } @@ -8819,4 +8465,44 @@ 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/812fa394/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()); + } }