stoty commented on a change in pull request #1096:
URL: https://github.com/apache/phoenix/pull/1096#discussion_r559588560



##########
File path: 
phoenix-core/src/it/java/org/apache/hadoop/hbase/regionserver/wal/WALReplayWithIndexWritesAndCompressedWALIT.java
##########
@@ -155,6 +156,15 @@ protected void startCluster() throws Exception {
 
   @After
   public void tearDown() throws Exception {
+    try {
+      CompatUtil.confirmStoreRefCountLeak(UTIL.getAdmin());
+    } catch (IOException e) {
+      LOGGER.error("StoreFile refCount is leaked", e);
+      UTIL.shutdownMiniHBaseCluster();

Review comment:
       Should just put these statements into a `finally`  instead.

##########
File path: 
phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java
##########
@@ -101,8 +100,9 @@ public void initTable() throws Exception {
     }
 
     @After
-    public void cleanUp(){
-        tableName=null;
+    public void cleanUp() throws Exception {
+        confirmStoreRefCountLeak();
+        tableName = null;

Review comment:
       switch the statement order ?

##########
File path: 
phoenix-core/src/it/java/org/apache/phoenix/end2end/BackwardCompatibilityIT.java
##########
@@ -110,6 +117,18 @@ public synchronized void doSetup() throws Exception {
     
     @After
     public void cleanUpAfterTest() throws Exception {
+        try {
+            CompatUtil.confirmStoreRefCountLeak(
+                hbaseTestUtil.getAdmin());
+        } catch (IOException e) {
+            LOGGER.error("StoreFile refCount is leaked", e);
+            try {

Review comment:
       this can also go into finally.

##########
File path: 
phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolTimeRangeIT.java
##########
@@ -226,7 +225,8 @@ public void tickTime() {
     }
 
     @AfterClass
-    public static synchronized void teardown() {
+    public static synchronized void teardown() throws Exception {
+        confirmStoreRefCountLeak();

Review comment:
       this should also be
   ```
   try {
     confirmStore...
   } catch (Exception e) {
     Assert.fail()}
   } finally {
     tearDownMiniCluster...
   }
   ```

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/InListIT.java
##########
@@ -73,7 +74,8 @@ public void setup() throws Exception {
     }
 
     @After
-    public void cleanUp() throws SQLException {
+    public void cleanUp() throws Exception {
+        confirmStoreRefCountLeak();
         deleteTenantData(descViewName);

Review comment:
       The should probably go into a finally as well.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to