You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/12/22 11:09:36 UTC
[GitHub] [iceberg] lirui-apache opened a new pull request #3790: Test: Make sure to delete temp folders
lirui-apache opened a new pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790
Use `FileUtils.deleteQuietly` to delete the temp folders created in `TestHiveMetastore`, `SparkTestBaseWithCatalog` and `TestCatalogTableLoader`. Use JUnit `TemporaryFolder` in `TestHiveCatalog`.
Closes #3750
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r776922757
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -272,7 +270,7 @@ public void testCreateTableCustomSortOrder() {
}
@Test
- public void testCreateNamespace() throws TException {
+ public void testCreateNamespace() throws Exception {
Review comment:
`hiveLocalDir` in this test is also not cleared. I'll just take care of that and revert irrelevant changes.
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778040932
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -220,6 +269,8 @@ private void initConf(HiveConf conf, int port) {
conf.set(HiveConf.ConfVars.METASTORE_TRY_DIRECT_SQL.varname, "false");
conf.set(HiveConf.ConfVars.METASTORE_DISALLOW_INCOMPATIBLE_COL_TYPE_CHANGES.varname, "false");
conf.set("iceberg.hive.client-pool-size", "2");
+ // hive 1.x requires a valid DN connection pool, use bonecp as it exposes a close method
+ conf.set("datanucleus.connectionPoolingType", "BONECP");
Review comment:
It's not an issue. BONECP is the default value. I just explicitly set it here in case the default value changes in the future.
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778806282
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
Sorry I just meant test results are not deterministic. A test may fail if it's sharing JVM and runs after some other tests. Then in another run, if it doesn't share JVM, or runs with different other tests, it can pass.
I'm not saying hive version is randomly chosen.
--
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
[GitHub] [iceberg] lirui-apache commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1008838111
Hi @pvary , do you think we can use shutdown hooks to delete the folders when JVM exits, so that we don't need all the reflections?
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r782164584
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
+ CONN_POOL.set(null);
+ if (connPool instanceof Closeable) {
+ try {
+ ((Closeable) connPool).close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close connection pool in TxnHandler", e);
+ }
+ }
+ OBJECT_STORE_PROP.set(null);
+ PersistenceManagerFactory pmf = OBJECT_STORE_PMF.get();
+ if (pmf != null) {
+ pmf.close();
+ }
+ if (hiveLocalDir != null && hiveLocalDir.exists()) {
+ try {
+ Path localDirPath = new Path(hiveLocalDir.getAbsolutePath());
+ FileSystem fs = Util.getFs(localDirPath, hiveConf);
+ Preconditions.checkState(fs.delete(localDirPath, true), "Failed to delete " + localDirPath);
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to delete local dir " + hiveLocalDir.getAbsolutePath(), e);
Review comment:
Done
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777438284
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
+ CONN_POOL.set(null);
+ if (connPool instanceof Closeable) {
+ try {
+ ((Closeable) connPool).close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close connection pool in TxnHandler", e);
+ }
+ }
+ OBJECT_STORE_PROP.set(null);
+ PersistenceManagerFactory pmf = OBJECT_STORE_PMF.get();
+ if (pmf != null) {
+ pmf.close();
+ }
+ if (hiveLocalDir != null && hiveLocalDir.exists()) {
+ try {
+ Path localDirPath = new Path(hiveLocalDir.getAbsolutePath());
+ FileSystem fs = Util.getFs(localDirPath, hiveConf);
+ Preconditions.checkState(fs.delete(localDirPath, true), "Failed to delete " + localDirPath);
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to delete local dir " + hiveLocalDir.getAbsolutePath(), e);
Review comment:
Why not just allow the exception to be propagated?
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777959623
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBaseWithCatalog.java
##########
@@ -44,9 +45,7 @@ public static void createWarehouse() throws IOException {
@AfterClass
public static void dropWarehouse() {
- if (warehouse != null && warehouse.exists()) {
- warehouse.delete();
- }
+ FileUtils.deleteQuietly(warehouse);
Review comment:
Using `Assert` here as you do in the new PR versions looks good to me
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777848628
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
+ CONN_POOL.set(null);
+ if (connPool instanceof Closeable) {
+ try {
+ ((Closeable) connPool).close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close connection pool in TxnHandler", e);
+ }
+ }
+ OBJECT_STORE_PROP.set(null);
+ PersistenceManagerFactory pmf = OBJECT_STORE_PMF.get();
+ if (pmf != null) {
+ pmf.close();
+ }
+ if (hiveLocalDir != null && hiveLocalDir.exists()) {
+ try {
+ Path localDirPath = new Path(hiveLocalDir.getAbsolutePath());
+ FileSystem fs = Util.getFs(localDirPath, hiveConf);
+ Preconditions.checkState(fs.delete(localDirPath, true), "Failed to delete " + localDirPath);
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to delete local dir " + hiveLocalDir.getAbsolutePath(), e);
Review comment:
Do you mean to allow this method to throw `IOException`? Then the signature needs update and will probably incur more changes. Will do that if it's OK.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r773880176
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBaseWithCatalog.java
##########
@@ -44,9 +45,7 @@ public static void createWarehouse() throws IOException {
@AfterClass
public static void dropWarehouse() {
- if (warehouse != null && warehouse.exists()) {
- warehouse.delete();
- }
+ FileUtils.deleteQuietly(warehouse);
Review comment:
This might hide unexpected exceptions which can indicate problems. I have preferred the original solution
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r776884219
##########
File path: flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/TestCatalogTableLoader.java
##########
@@ -26,6 +26,7 @@
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.util.Map;
+import org.apache.commons.io.FileUtils;
Review comment:
We don't use Apache commons in Iceberg. Can you replace this with something else?
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r776918351
##########
File path: flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/TestCatalogTableLoader.java
##########
@@ -26,6 +26,7 @@
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.util.Map;
+import org.apache.commons.io.FileUtils;
Review comment:
I noticed this class is being used in several other tests so thought it's all right. Guess I can use Hadoop `FileSystem` if that's OK. Alternatively, Guava has a similar functionality in `MoreFiles` but seems it's not included in the bundled jar at the moment.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778799532
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
If we are really using Hive 2.3.x and then Hive 1.2.1 randomly then this is a big issue. We should make this deterministic.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778053813
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
Review comment:
I think(hope) it does this automatically, but not a gradle expert myself. I remember comping to this conclusion when I have faced some ThreadLocal issues myself, but I might be mistaken
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777974537
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
Review comment:
So again, we face the issue that the HiveMetastore is not planned for stop/start (which is a shame), but IMHO we could/should not fix it here. We just have to use it in the way as it is planned:
- Create only one HMS instance per test class
- Make sure that the test infra is using different thread (or JVM) to run the test classes
- Remove other leftover resources which are not cleaned up (like the temp directory you correctly identified)
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r774466558
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -287,7 +285,11 @@ public void testCreateNamespace() throws TException {
AlreadyExistsException.class, "Namespace '" + namespace1 + "' already exists!", () -> {
catalog.createNamespace(namespace1);
});
- ImmutableMap newMeta = ImmutableMap.<String, String>builder()
+ String hiveLocalDir = temp.newFolder().toURI().toString();
+ if (hiveLocalDir.endsWith("/")) {
Review comment:
The `java.io.File::toURI` doc mentions:
```java
* <p> The exact form of the URI is system-dependent. If it can be
* determined that the file denoted by this abstract pathname is a
* directory, then the resulting URI will end with a slash.
```
Not sure if that guarantees a trailing slash, so I added the check to be safe.
And btw the trailing slash (if any) is removed when hive converts the string to a Hadoop Path object: https://github.com/apache/hadoop/blob/rel/release-2.7.3/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java#L237
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r773879591
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -112,6 +113,9 @@ public void start(HiveConf conf) {
public void start(HiveConf conf, int poolSize) {
try {
this.hiveLocalDir = createTempDirectory("hive", asFileAttribute(fromString("rwxrwxrwx"))).toFile();
+ // Ideally we can delete the folder in stop(), but tests would fail if we do that, probably because we are not
Review comment:
I would prefer to fix or at least find the underlying issue.
Otherwise the files which are carried over will cause issues which are really hard to debug
--
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
[GitHub] [iceberg] pvary commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1009988212
Thanks @lirui-apache!
I will give a few days for the other reviewers to chime in, if they want. If there are no objections then I will merge this later this week.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777437898
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
+ CONN_POOL.set(null);
+ if (connPool instanceof Closeable) {
+ try {
+ ((Closeable) connPool).close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close connection pool in TxnHandler", e);
+ }
+ }
+ OBJECT_STORE_PROP.set(null);
+ PersistenceManagerFactory pmf = OBJECT_STORE_PMF.get();
Review comment:
These should be cleaned up immediately when there is a configuration change in the connection pool URI.
Maybe our tests are not setting the connection pool URI correctly?
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777437337
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
`HMSHandler.threadLocalConf` is supposed to be removed here which is called in the `shutdown`:
https://github.com/apache/hive/blob/branch-2.3/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java#L7287
You might be right that the `threadLocalTxn` is not cleaned up, but I would guess this does not affect our test yet, as we do not test transactional tables.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777444215
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
+ CONN_POOL.set(null);
+ if (connPool instanceof Closeable) {
+ try {
+ ((Closeable) connPool).close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close connection pool in TxnHandler", e);
+ }
+ }
+ OBJECT_STORE_PROP.set(null);
+ PersistenceManagerFactory pmf = OBJECT_STORE_PMF.get();
+ if (pmf != null) {
+ pmf.close();
+ }
+ if (hiveLocalDir != null && hiveLocalDir.exists()) {
+ try {
+ Path localDirPath = new Path(hiveLocalDir.getAbsolutePath());
+ FileSystem fs = Util.getFs(localDirPath, hiveConf);
+ Preconditions.checkState(fs.delete(localDirPath, true), "Failed to delete " + localDirPath);
Review comment:
Why not just use the same pattern than the other changes?
```
Assert.assertTrue("Failed to delete " + localDirPath, fs.delete(localDirPath, true));
```
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778054161
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
+ CONN_POOL.set(null);
+ if (connPool instanceof Closeable) {
+ try {
+ ((Closeable) connPool).close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close connection pool in TxnHandler", e);
+ }
+ }
+ OBJECT_STORE_PROP.set(null);
+ PersistenceManagerFactory pmf = OBJECT_STORE_PMF.get();
+ if (pmf != null) {
+ pmf.close();
+ }
+ if (hiveLocalDir != null && hiveLocalDir.exists()) {
+ try {
+ Path localDirPath = new Path(hiveLocalDir.getAbsolutePath());
+ FileSystem fs = Util.getFs(localDirPath, hiveConf);
+ Preconditions.checkState(fs.delete(localDirPath, true), "Failed to delete " + localDirPath);
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to delete local dir " + hiveLocalDir.getAbsolutePath(), e);
Review comment:
I would vote for extending the method throw clause.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r773878033
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -287,7 +285,11 @@ public void testCreateNamespace() throws TException {
AlreadyExistsException.class, "Namespace '" + namespace1 + "' already exists!", () -> {
catalog.createNamespace(namespace1);
});
- ImmutableMap newMeta = ImmutableMap.<String, String>builder()
+ String hiveLocalDir = temp.newFolder().toURI().toString();
+ if (hiveLocalDir.endsWith("/")) {
Review comment:
Is the last character of the URI changing depending on the system?
--
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
[GitHub] [iceberg] pvary commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-999571704
@lirui-apache: Thanks for the PR. Did a quick cursory look, but sadly did not have time to a full review, as I have plenty to do and will be out on PTO till the end of the year.
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777849627
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -220,6 +269,8 @@ private void initConf(HiveConf conf, int port) {
conf.set(HiveConf.ConfVars.METASTORE_TRY_DIRECT_SQL.varname, "false");
conf.set(HiveConf.ConfVars.METASTORE_DISALLOW_INCOMPATIBLE_COL_TYPE_CHANGES.varname, "false");
conf.set("iceberg.hive.client-pool-size", "2");
+ // hive 1.x requires a valid DN connection pool, use bonecp as it exposes a close method
+ conf.set("datanucleus.connectionPoolingType", "BONECP");
Review comment:
I used gradle `dependencies` and checked `testRuntimeClasspath` of spark 2.4. And the hive version is `org.spark-project.hive:hive-exec:1.2.1.spark2`. So I suppose we're already running with 1.x
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778035743
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
I guess whether threads are reused depends on the testing framework. I can revert the clean up code to reproduce the test failures (did that earlier but seems CI report is lost after force push :slightly_frowning_face: )
--
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
[GitHub] [iceberg] lirui-apache commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1011831777
@pvary Thanks for reviewing and merging the PR!
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r782164378
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
So we reverted this part of the change, for the "global" derby dir approach
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777426801
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -287,7 +285,11 @@ public void testCreateNamespace() throws TException {
AlreadyExistsException.class, "Namespace '" + namespace1 + "' already exists!", () -> {
catalog.createNamespace(namespace1);
});
- ImmutableMap newMeta = ImmutableMap.<String, String>builder()
+ String hiveLocalDir = temp.newFolder().toURI().toString();
+ if (hiveLocalDir.endsWith("/")) {
Review comment:
I would remove this change in the name of simplicity until we find some concrete use-cases where the tests are failing
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r773876120
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -272,7 +270,7 @@ public void testCreateTableCustomSortOrder() {
}
@Test
- public void testCreateNamespace() throws TException {
+ public void testCreateNamespace() throws Exception {
Review comment:
I like this change, we can get merged independently of the other parts.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777426801
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -287,7 +285,11 @@ public void testCreateNamespace() throws TException {
AlreadyExistsException.class, "Namespace '" + namespace1 + "' already exists!", () -> {
catalog.createNamespace(namespace1);
});
- ImmutableMap newMeta = ImmutableMap.<String, String>builder()
+ String hiveLocalDir = temp.newFolder().toURI().toString();
+ if (hiveLocalDir.endsWith("/")) {
Review comment:
I would remove this change in the nape of simplicity until we find some concrete use-cases where the tests are failing
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778054497
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -220,6 +269,8 @@ private void initConf(HiveConf conf, int port) {
conf.set(HiveConf.ConfVars.METASTORE_TRY_DIRECT_SQL.varname, "false");
conf.set(HiveConf.ConfVars.METASTORE_DISALLOW_INCOMPATIBLE_COL_TYPE_CHANGES.varname, "false");
conf.set("iceberg.hive.client-pool-size", "2");
+ // hive 1.x requires a valid DN connection pool, use bonecp as it exposes a close method
+ conf.set("datanucleus.connectionPoolingType", "BONECP");
Review comment:
I would just keep the default if it does not hurt
--
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
[GitHub] [iceberg] lirui-apache commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1005444824
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.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r781991103
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -233,8 +253,8 @@ private void setupMetastoreDB(String dbURL) throws SQLException, IOException {
}
}
- private String getDerbyPath() {
- File metastoreDB = new File(hiveLocalDir, "metastore_db");
+ private static String getDerbyPath() {
Review comment:
nit: If the `HIVE_LOCAL_DIR` is static, we might get nicer code if we create a static `DERBY_PATH` 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.
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
[GitHub] [iceberg] lirui-apache commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1009868366
Thanks @pvary for the review and suggestions. I added a static field for derby path.
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r774471053
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBaseWithCatalog.java
##########
@@ -44,9 +45,7 @@ public static void createWarehouse() throws IOException {
@AfterClass
public static void dropWarehouse() {
- if (warehouse != null && warehouse.exists()) {
- warehouse.delete();
- }
+ FileUtils.deleteQuietly(warehouse);
Review comment:
Do you mean we should not suppress exceptions if the folder can't be deleted? Then I guess we can use `FileUtils::forceDelete`, which has the following in java doc:
```java
* Deletes a file. If file is a directory, delete it and all sub-directories.
* <p>
* The difference between File.delete() and this method are:
* <ul>
* <li>A directory to be deleted does not have to be empty.</li>
* <li>You get exceptions when a file or directory cannot be deleted.
* (java.io.File methods returns a boolean)</li>
* </ul>
```
The problem with the original solution is it can't delete this folder.
--
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
[GitHub] [iceberg] lirui-apache commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1005675333
>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.
Yes. I think that's also a viable solution. And we can go back with a shutdown hook like the 1st version of this PR.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777440995
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -220,6 +269,8 @@ private void initConf(HiveConf conf, int port) {
conf.set(HiveConf.ConfVars.METASTORE_TRY_DIRECT_SQL.varname, "false");
conf.set(HiveConf.ConfVars.METASTORE_DISALLOW_INCOMPATIBLE_COL_TYPE_CHANGES.varname, "false");
conf.set("iceberg.hive.client-pool-size", "2");
+ // hive 1.x requires a valid DN connection pool, use bonecp as it exposes a close method
+ conf.set("datanucleus.connectionPoolingType", "BONECP");
Review comment:
Would this allow the tests to run with Hive 1.x 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.
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777431096
##########
File path: flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/TestCatalogTableLoader.java
##########
@@ -58,9 +60,11 @@ public static void createWarehouse() throws IOException {
}
@AfterClass
- public static void dropWarehouse() {
+ public static void dropWarehouse() throws IOException {
if (warehouse != null && warehouse.exists()) {
- warehouse.delete();
+ Path warehousePath = new Path(warehouse.getAbsolutePath());
+ FileSystem fs = warehousePath.getFileSystem(hiveConf);
+ Assert.assertTrue("Failed to delete " + warehousePath, fs.delete(warehousePath, true));
Review comment:
Taking a fresh look at the code here, I think we might want to do this cleanup in the `TestHiveMetastore.stop()`.
We already clean up the warehouse root in `TestHiveMetastore.stop()`, we might want to clean up the data in the stop as well.
What do you think?
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r776884301
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -272,7 +270,7 @@ public void testCreateTableCustomSortOrder() {
}
@Test
- public void testCreateNamespace() throws TException {
+ public void testCreateNamespace() throws Exception {
Review comment:
+1
If this is a separate fix, then let's move it to a different PR.
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777845827
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
Regarding `threadLocalTxn`, it's used when HMSHandler tries to acquire locks, so I think it also needs to be cleaned.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r776884428
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
What are you doing here?
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r776884317
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -82,6 +88,33 @@
.orNoop()
.buildStatic();
+ private static final ThreadLocal<?> HMS_HANDLER_THREAD_LOCAL_CONF =
+ (ThreadLocal<?>) DynFields.builder()
+ .hiddenImpl(HiveMetaStore.HMSHandler.class, "threadLocalConf")
+ .buildStatic()
+ .get();
Review comment:
Indentation is off.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777437759
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
Review comment:
These should be cleaned up immediately when there is a configuration change in the connection pool URI.
Maybe our tests are not setting the connection pool URI correctly?
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r773878389
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -296,7 +298,7 @@ public void testCreateNamespace() throws TException {
catalog.createNamespace(namespace2, newMeta);
Database database2 = metastoreClient.getDatabase(namespace2.toString());
Assert.assertEquals("There no same location for db and namespace",
- database2.getLocationUri(), hiveLocalDir);
+ hiveLocalDir, database2.getLocationUri());
Review comment:
nit: Please fix the indentation.
Why did we change this?
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r774467865
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -296,7 +298,7 @@ public void testCreateNamespace() throws TException {
catalog.createNamespace(namespace2, newMeta);
Database database2 = metastoreClient.getDatabase(namespace2.toString());
Assert.assertEquals("There no same location for db and namespace",
- database2.getLocationUri(), hiveLocalDir);
+ hiveLocalDir, database2.getLocationUri());
Review comment:
IIUC, `hiveLocalDir` should be the **expected** value, while `database2.getLocationUri()` should be the **actual** value in this assertion.
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r774468767
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -112,6 +113,9 @@ public void start(HiveConf conf) {
public void start(HiveConf conf, int poolSize) {
try {
this.hiveLocalDir = createTempDirectory("hive", asFileAttribute(fromString("rwxrwxrwx"))).toFile();
+ // Ideally we can delete the folder in stop(), but tests would fail if we do that, probably because we are not
Review comment:
OK, I'll look into that.
--
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
[GitHub] [iceberg] pvary commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1011822310
Since there were no new comments, merged to master.
Thanks for the patch @lirui-apache!
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777816572
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
I think spark 2.4 depends on `org.spark-project.hive:hive-exec:1.2.1.spark2` which is based on hive-1.2.1 and which doesn't clear `threadLocalConf ` in HMSHandler::shutdown method, right?
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778757919
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
I would be surprised if the tests would use 1.2.1 HMSHandler. When the error happens the stack traces shows this:
```
at org.apache.hadoop.hive.metastore.txn.TxnHandler.cleanupRecords(TxnHandler.java:1888)
at org.apache.hadoop.hive.metastore.AcidEventListener.onDropDatabase(AcidEventListener.java:51)
at org.apache.hadoop.hive.metastore.MetaStoreListenerNotifier$13.notify(MetaStoreListenerNotifier.java:69)
```
This code is not present in 1.2.1 Hive
--
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
[GitHub] [iceberg] lirui-apache commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1003290854
@rdblue Thanks for the review. PR updated accordingly. Please have another look.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777971278
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
I do not think that the threads get reused between tests. If ThreadLocals would be problematic, we should have faced the issues much sooner. Do we have tests where a TestHiveMetastore instance is created/recreated between test methods?
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777849845
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
+ CONN_POOL.set(null);
+ if (connPool instanceof Closeable) {
+ try {
+ ((Closeable) connPool).close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close connection pool in TxnHandler", e);
+ }
+ }
+ OBJECT_STORE_PROP.set(null);
+ PersistenceManagerFactory pmf = OBJECT_STORE_PMF.get();
+ if (pmf != null) {
+ pmf.close();
+ }
+ if (hiveLocalDir != null && hiveLocalDir.exists()) {
+ try {
+ Path localDirPath = new Path(hiveLocalDir.getAbsolutePath());
+ FileSystem fs = Util.getFs(localDirPath, hiveConf);
+ Preconditions.checkState(fs.delete(localDirPath, true), "Failed to delete " + localDirPath);
Review comment:
OK will make them consistent.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777978866
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -220,6 +269,8 @@ private void initConf(HiveConf conf, int port) {
conf.set(HiveConf.ConfVars.METASTORE_TRY_DIRECT_SQL.varname, "false");
conf.set(HiveConf.ConfVars.METASTORE_DISALLOW_INCOMPATIBLE_COL_TYPE_CHANGES.varname, "false");
conf.set("iceberg.hive.client-pool-size", "2");
+ // hive 1.x requires a valid DN connection pool, use bonecp as it exposes a close method
+ conf.set("datanucleus.connectionPoolingType", "BONECP");
Review comment:
Why did we not face this issue before? What is new?
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778052306
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
IMHO, reproducing the specific issue would be the best. And only fix the specific issue. Fixing everything would clutter the code seriously, and nobody would understand why all of these fixes are there. (I am hoping here that we would not face the other issues in the near future)
--
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
[GitHub] [iceberg] pvary merged pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary merged pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778791284
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
I meant spark 2.4 relies on hive 1.2.1. But in the latest CI, spark2 passed. I think it's not deterministic due to how tests are scheduled and how JVMs are reused. Seems we can force the problem to reproduce. E.g. I added this test case in `TestIdentityPartitionData` in spark 2.4 to do a restart of hive metastore:
```java
@Test
public void test() throws Exception {
testFullProjection();
stopMetastoreAndSpark();
startMetastoreAndSpark();
testFullProjection();
}
```
And run the test with `./gradlew -DsparkVersions=2.4 :iceberg-spark:iceberg-spark2:test --tests org.apache.iceberg.spark.source.TestIdentityPartitionData`. The test would fail with:
```
org.apache.iceberg.spark.source.TestIdentityPartitionData > test[format = parquet, vectorized = false] FAILED
java.lang.RuntimeException: Cannot start TestHiveMetastore
at org.apache.iceberg.hive.TestHiveMetastore.start(TestHiveMetastore.java:134)
at org.apache.iceberg.hive.TestHiveMetastore.start(TestHiveMetastore.java:97)
at org.apache.iceberg.spark.SparkTestBase.startMetastoreAndSpark(SparkTestBase.java:57)
at org.apache.iceberg.spark.source.TestIdentityPartitionData.test(TestIdentityPartitionData.java:143)
Caused by:
javax.jdo.JDOException: Exception thrown when executing query
NestedThrowables:
java.sql.SQLException: Container 2,177 not found.
at org.datanucleus.api.jdo.NucleusJDOHelper.getJDOExceptionForNucleusException(NucleusJDOHelper.java:596)
at org.datanucleus.api.jdo.JDOQuery.execute(JDOQuery.java:252)
at org.apache.hadoop.hive.metastore.ObjectStore.getMRole(ObjectStore.java:3531)
at org.apache.hadoop.hive.metastore.ObjectStore.addRole(ObjectStore.java:3221)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.apache.hadoop.hive.metastore.RawStoreProxy.invoke(RawStoreProxy.java:114)
at com.sun.proxy.$Proxy16.addRole(Unknown Source)
at org.apache.hadoop.hive.metastore.HiveMetaStore$HMSHandler.createDefaultRoles_core(HiveMetaStore.java:656)
at org.apache.hadoop.hive.metastore.HiveMetaStore$HMSHandler.createDefaultRoles(HiveMetaStore.java:648)
at org.apache.hadoop.hive.metastore.HiveMetaStore$HMSHandler.init(HiveMetaStore.java:462)
at org.apache.hadoop.hive.metastore.HiveMetaStore$HMSHandler.<init>(HiveMetaStore.java:419)
at org.apache.hadoop.hive.metastore.HiveMetaStore$HMSHandler.<init>(HiveMetaStore.java:412)
```
And the stack trace matches hive 1.2.1:
https://github.com/apache/hive/blob/release-1.2.1/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java#L462
--
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
[GitHub] [iceberg] pvary commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1008846328
> Hi @pvary , do you think we can use shutdown hooks to delete the folders when JVM exits, so that we don't need all the reflections?
If we can make that working I would like it much more.
Thanks for working this and persisting in creating the best solution!
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r774467865
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -296,7 +298,7 @@ public void testCreateNamespace() throws TException {
catalog.createNamespace(namespace2, newMeta);
Database database2 = metastoreClient.getDatabase(namespace2.toString());
Assert.assertEquals("There no same location for db and namespace",
- database2.getLocationUri(), hiveLocalDir);
+ hiveLocalDir, database2.getLocationUri());
Review comment:
IIUC, `hiveLocalDir` should be the **expected** value, while `database2.getLocationUri()` should be the actual **value** in this assertion.
--
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
[GitHub] [iceberg] lirui-apache commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1000829379
The latest patch deletes the temp folder between test class, so the test failures are reproduced. I'll update to clear more static fields.
--
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
[GitHub] [iceberg] lirui-apache commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1001855663
Hey @pvary , please have another look when you have time. Basically I just identify some suspicious static fields and clear/close them.
--
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
[GitHub] [iceberg] pvary commented on pull request #3790: Test: Make sure to delete temp folders
Posted by 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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777817983
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
Review comment:
`TxnHandler::setupJdbcConnectionPool` doesn't do anything if `connPool` is already set:
https://github.com/apache/hive/blob/release-1.2.1/metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java#L1862
Besides, I also want to close it in case there's any resource leak.
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777847623
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
+ CONN_POOL.set(null);
+ if (connPool instanceof Closeable) {
+ try {
+ ((Closeable) connPool).close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close connection pool in TxnHandler", e);
+ }
+ }
+ OBJECT_STORE_PROP.set(null);
+ PersistenceManagerFactory pmf = OBJECT_STORE_PMF.get();
Review comment:
Yeah it seems `OBJECT_STORE_PROP` and `OBJECT_STORE_PMF` will be cleaned up when connection URI changes. I took a conservative approach to clean them anyway because I thought it does no harm and is safer than relying on hive implementation details (especially when we have multiple versions to support). If you prefer not to do it, I'll revert the change.
--
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
[GitHub] [iceberg] lirui-apache commented on pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#issuecomment-1005438345
Hi @pvary , please take a look at the latest CI to find the reproduced failures.
--
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r778039885
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +171,35 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
+ Object connPool = CONN_POOL.get();
Review comment:
I didn't find gradle/junit configurations to force test classes to run in different threads. We can go with different JVMs, at the cost of longer test time. Actually that's what I did when I worked on the hive integration in flink. I believe it can be achieved by setting `forkEvery` to 1.
--
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
[GitHub] [iceberg] pvary commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r777443197
##########
File path: flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/TestCatalogTableLoader.java
##########
@@ -58,9 +60,11 @@ public static void createWarehouse() throws IOException {
}
@AfterClass
- public static void dropWarehouse() {
+ public static void dropWarehouse() throws IOException {
if (warehouse != null && warehouse.exists()) {
- warehouse.delete();
+ Path warehousePath = new Path(warehouse.getAbsolutePath());
+ FileSystem fs = warehousePath.getFileSystem(hiveConf);
+ Assert.assertTrue("Failed to delete " + warehousePath, fs.delete(warehousePath, true));
Review comment:
Ok.. this is for other warehouses 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.
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
[GitHub] [iceberg] lirui-apache commented on a change in pull request #3790: Test: Make sure to delete temp folders
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on a change in pull request #3790:
URL: https://github.com/apache/iceberg/pull/3790#discussion_r776918882
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java
##########
@@ -144,13 +177,33 @@ public void stop() {
if (executorService != null) {
executorService.shutdown();
}
- if (hiveLocalDir != null) {
- hiveLocalDir.delete();
- }
if (baseHandler != null) {
baseHandler.shutdown();
}
METASTORE_THREADS_SHUTDOWN.invoke();
+ HMS_HANDLER_THREAD_LOCAL_CONF.remove();
+ HMS_HANDLER_THREAD_LOCAL_TXN.remove();
Review comment:
I'm trying to remove these thread local fields in HMSHandler, so that they won't get reused in follow-up tests.
--
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