commit 431197d415e3930767499706a84a42af9fa66717 Author: Vihang Karajgaonkar Date: Mon Apr 3 15:57:31 2017 -0700 HIVE-16213 : ObjectStore can leak Queries when rollbackTransaction throws an exception 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 6b217516bc74c612348b8edeca077dfbdbdb1a40..c7d224911f05e1f7dbab2acd58577e3c30f98372 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 @@ 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 @@ private MDatabase getMDatabase(String name) throws NoSuchObjectException { 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 boolean dropDatabase(String dbname) throws NoSuchObjectException, MetaExc } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return success; } @@ -858,12 +846,7 @@ public boolean dropDatabase(String dbname) throws NoSuchObjectException, MetaExc } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return databases; } @@ -883,12 +866,7 @@ public boolean dropDatabase(String dbname) throws NoSuchObjectException, MetaExc databases = new ArrayList((Collection) 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 Type getType(String typeName) { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return type; } @@ -985,12 +958,7 @@ public boolean dropType(String typeName) { 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 Table getTable(String dbName, String tableName) throws MetaException { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return tbls; } @@ -1271,12 +1234,7 @@ private int getObjectCount(String fieldName, String objName) { 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 @@ private int getObjectCount(String fieldName, String objName) { } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return metas; } @@ -1405,12 +1358,7 @@ private AttachedMTableInfo getMTable(String db, String table, boolean retrieveCD } commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } nmtbl.mtbl = mtbl; return nmtbl; @@ -1453,15 +1401,10 @@ private MTable getMTable(String db, String table) { } 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 @@ private MPartition getMPartition(String dbName, String tableName, List p } } } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return ret; } @@ -2216,10 +2154,7 @@ private boolean dropPartitionCommon(MPartition part) throws NoSuchObjectExceptio success = commitTransaction(); return parts; } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } } @@ -2321,6 +2256,7 @@ public Partition getPartitionWithAuth(String dbName, String tblName, for (Iterator i = names.iterator(); i.hasNext();) { pns.add((String) i.next()); } + if (query != null) { query.closeAll(); } @@ -2415,10 +2351,7 @@ private Collection getPartitionPsQueryResults(String dbName, String tableName, } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return partitions; } @@ -2440,10 +2373,7 @@ private Collection getPartitionPsQueryResults(String dbName, String tableName, } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return partitionNames; } @@ -3209,12 +3139,7 @@ private String makeParameterDeclarationStringObj(Map params) { 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 @@ private String makeParameterDeclarationStringObj(Map params) { 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 @@ private void removeUnusedColumnDescriptor(MColumnDescriptor oldCD) { success = commitTransaction(); LOG.debug("successfully deleted a CD in removeUnusedColumnDescriptor"); } finally { - if (!success) { - rollbackTransaction(); - } - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } } @@ -3571,12 +3488,7 @@ private boolean constraintNameAlreadyExists(String name) { 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 @@ private MIndex getMIndex(String dbName, String originalTblName, String indexName pm.retrieve(midx); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return midx; } @@ -3892,12 +3799,7 @@ private Index convertToIndex(MIndex mIndex) throws MetaException { return indexes; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -3924,12 +3826,7 @@ private Index convertToIndex(MIndex mIndex) throws MetaException { } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } return pns; } @@ -4052,12 +3949,7 @@ private MRoleMap getMSecurityUserRoleMap(String userName, PrincipalType principa pm.retrieve(mRoleMember); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return mRoleMember; } @@ -4126,11 +4018,7 @@ public boolean removeRole(String roleName) throws MetaException, } success = commitTransaction(); } finally { - if (!success) { - rollbackTransaction(); - } - - queryWrapper.close(); + rollbackAndCleanup(success, queryWrapper); } return success; } @@ -4200,12 +4088,7 @@ private void getAllRoleAncestors(Set processedRoleNames, List 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 @@ private void getAllRoleAncestors(Set processedRoleNames, List mRoleMemebership = (List) 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 @@ private MRole getMRole(String roleName) { pm.retrieve(mrole); commited = commitTransaction(); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return mrole; } @@ -4332,12 +4209,7 @@ private MRole getMRole(String roleName) { success = commitTransaction(); return roleNames; } finally { - if (!success) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(success, query); } } @@ -5163,12 +5035,7 @@ public boolean revokePrivileges(PrivilegeBag privileges, boolean grantOption) 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 boolean revokePrivileges(PrivilegeBag privileges, boolean grantOption) userNameDbPriv.addAll(mPrivs); } } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } return userNameDbPriv; } @@ -5264,12 +5126,7 @@ public boolean revokePrivileges(PrivilegeBag privileges, boolean grantOption) commited = commitTransaction(); return convertGlobal(userNameDbPriv); } finally { - if (!commited) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(commited, query); } } @@ -5312,12 +5169,7 @@ public boolean revokePrivileges(PrivilegeBag privileges, boolean grantOption) 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 boolean revokePrivileges(PrivilegeBag privileges, boolean grantOption) 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 boolean revokePrivileges(PrivilegeBag privileges, boolean grantOption) 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 boolean revokePrivileges(PrivilegeBag privileges, boolean grantOption) 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 boolean revokePrivileges(PrivilegeBag privileges, boolean grantOption) 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 void dropPartitionAllColumnGrantsNoTxn( private List listDatabaseGrants(String dbName, QueryWrapper queryWrapper) { dbName = HiveStringUtils.normalizeIdentifier(dbName); boolean success = false; - try { LOG.debug("Executing listDatabaseGrants"); @@ -5695,12 +5526,7 @@ private void dropPartitionGrantsNoTxn(String dbName, String tableName, List partName) throws In 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 long executeJDOQLUpdate(String queryStr) { return -1; } } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6542,12 +6302,7 @@ public long executeJDOQLUpdate(String queryStr) { return null; } } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6658,12 +6413,7 @@ public UpdateMDatabaseURIRetVal updateMDatabaseURI(URI oldLoc, URI newLoc, boole } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6751,12 +6501,7 @@ public UpdatePropURIRetVal updateTblPropURI(URI oldLoc, URI newLoc, String tblPr } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6790,12 +6535,7 @@ public UpdatePropURIRetVal updateMStorageDescriptorTblPropURI(URI oldLoc, URI ne } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6886,12 +6626,7 @@ public UpdateMStorageDescriptorTblURIRetVal updateMStorageDescriptorTblURI(URI o } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -6968,12 +6703,7 @@ public UpdateSerdeURIRetVal updateSerdeURI(URI oldLoc, URI newLoc, String serdeP } return retVal; } finally { - if (!committed) { - rollbackTransaction(); - } - if (query != null) { - query.closeAll(); - } + rollbackAndCleanup(committed, query); } } @@ -7184,7 +6914,6 @@ public boolean updatePartitionColumnStatistics(ColumnStatistics colStats, ListanyObject()); + } }