[GitHub] [iceberg] pvary commented on pull request #3790: Test: Make sure to delete temp folders

[GitBox <gi...@apache.org>]

pvary commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1005646336


   > My observation is `HMSHandler::threadLocalConf`, `HMSHandler::threadLocalTxn` and `TxnHandler::connPool` need to be cleaned up. We probably don't have to worry about `ObjectStore::prop` and `ObjectStore::pmf`, but again, I'd prefer to clean them anyway to be safe.
   > 
   > Alternatively, we can disable JVM sharing between test classes.
   
   Thanks for the repro!
   I see that the issue is that the derby used by the TxnHandler throws an error when it is trying to clean up the database related items in the HMS DB. Did we see these errors in any other place then in the `AcidEventListener`?
   
   The `AcidEventListener` creates a new TxnHandler every time when the methods are called with this code:
   https://github.com/apache/hive/blob/rel/release-2.3.9/metastore/src/java/org/apache/hadoop/hive/metastore/AcidEventListener.java#L71-L93
   ```
     private TxnStore getTxnHandler() {
       boolean hackOn = HiveConf.getBoolVar(hiveConf, HiveConf.ConfVars.HIVE_IN_TEST) ||
           HiveConf.getBoolVar(hiveConf, HiveConf.ConfVars.HIVE_IN_TEZ_TEST);
       String origTxnMgr = null;
       boolean origConcurrency = false;
   
       // Since TxnUtils.getTxnStore calls TxnHandler.setConf -> checkQFileTestHack -> TxnDbUtil.setConfValues,
       // which may change the values of below two entries, we need to avoid pulluting the original values
       if (hackOn) {
         origTxnMgr = hiveConf.getVar(HiveConf.ConfVars.HIVE_TXN_MANAGER);
         origConcurrency = hiveConf.getBoolVar(HiveConf.ConfVars.HIVE_SUPPORT_CONCURRENCY);
       }
   
       txnHandler = TxnUtils.getTxnStore(hiveConf);
   
       // Set them back
       if (hackOn) {
         hiveConf.setVar(HiveConf.ConfVars.HIVE_TXN_MANAGER, origTxnMgr);
         hiveConf.setBoolVar(HiveConf.ConfVars.HIVE_SUPPORT_CONCURRENCY, origConcurrency);
       }
   
       return txnHandler;
     }
   ```
   
   This is definitely related to the `TxnHandler::connPool` as you have correctly identified. I think if we close and set the connPool to null, then we can fix this particular issue.
   
   I would guess that the `HMSHandler::threadLocalTxn` is not related with this specific issue. Might have the same issue as above if we try to use it, but I would guess that cleaning up the connection pool would solve this issue as well, as the TxnHandler itself is mostly stateless.
   
   I think `HMSHandler::threadLocalConf` is unrelated to the above and can cause issues when initialising the default databases, roles etc. This could be solved by this in `TestHiveMetastore::newThriftServer`:
   ```
       baseHandler = HMS_HANDLER_CTOR.newInstance("new db based metaserver", serverConf, false);
       baseHandler.setConf(serverConf);
       baseHandler.init();
   ```
   Basically the `setConf` fixes the configuration issue, and calling the `init()` after `setConf` ensures that it already uses the correct configuration.
   
   If I understand correctly all of the issues are caused by using a different derby root directory for the HMS instances. Before your fix we were trying to set different root directory for the derby, but effectively we were using the first one used for this thread. And since the files were not removed we were not having any issues. After the fix the directories were removed and this caused issues when we tried to read the derby files.
   
   So an alternative solution could be reusing the same derby db between tests, and deleting the data between the tests, and deleting the files only at the JVM exit. In this case we would not need any magic in the test code.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org