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 2020/09/08 13:14:26 UTC
[GitHub] [iceberg] openinx opened a new pull request #1433: Fix the bug that clientPool#close will block forever when newClient encountered an exception.
openinx opened a new pull request #1433:
URL: https://github.com/apache/iceberg/pull/1433
When preparing demo for flink + hive metastore, I created a hive catalog with a random port for metastore, for example:
```sql
create catalog hive_catalog with(
'type'='iceberg',
'catalog-type'='hive',
'uri'='thrift://localhost:3234',
'clients'='1',
'property-version'='1'
);
```
Then I switch to use the `hive_catalog` and execute sql command `show databases` to show all the databases, I found that the sql command was blocked.
After digging into the code, I find that it's a bug in `ClientPool`, saying if encountering an exception when `newClient`, the upper layer (Flink SQL client) will try to close all the resources from `ClientPool` by calling `ClientPool#close`, finally it will be blocked in close method like following:
```
"main" #1 prio=5 os_prio=31 tid=0x00007fc853802800 nid=0x1d03 in Object.wait() [0x000070000ad92000]
java.lang.Thread.State: TIMED_WAITING (on object monitor)
at java.lang.Object.wait(Native Method)
at org.apache.iceberg.hive.ClientPool.close(ClientPool.java:110)
- locked <0x0000000771b203c8> (a java.lang.Object)
at org.apache.iceberg.hive.HiveCatalog.close(HiveCatalog.java:480)
at org.apache.iceberg.flink.FlinkCatalog.close(FlinkCatalog.java:120)
at org.apache.flink.table.client.gateway.local.ExecutionContext$$Lambda$503/1760443245.accept(Unknown Source)
at java.util.HashMap$Values.forEach(HashMap.java:981)
at org.apache.flink.table.client.gateway.local.ExecutionContext.lambda$close$2(ExecutionContext.java:307)
at org.apache.flink.table.client.gateway.local.ExecutionContext$$Lambda$500/1972628089.run(Unknown Source)
at org.apache.flink.table.client.gateway.local.ExecutionContext.wrapClassLoader(ExecutionContext.java:264)
at org.apache.flink.table.client.gateway.local.ExecutionContext.close(ExecutionContext.java:307)
at org.apache.flink.table.client.gateway.local.LocalExecutor.closeSession(LocalExecutor.java:244)
at org.apache.flink.table.client.SqlClient.start(SqlClient.java:116)
at org.apache.flink.table.client.SqlClient.main(SqlClient.java:201)
```
The patch fixed this bug and attached an unit test to reproduce the case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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] RussellSpitzer commented on a change in pull request #1433: Fix the bug that clientPool#close will block forever when newClient encountered an exception.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1433:
URL: https://github.com/apache/iceberg/pull/1433#discussion_r484982190
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -114,8 +114,9 @@ private C get() throws InterruptedException {
if (!clients.isEmpty()) {
return clients.removeFirst();
} else if (currentSize < poolSize) {
+ C client = newClient();
currentSize += 1;
Review comment:
This is real issue here right? We have the possibility of incrementing current size even if "newClient" failed in the old code? If so this seems like the right fix 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.
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 #1433: Fix the bug that clientPool#close will block forever when newClient encountered an exception.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1433:
URL: https://github.com/apache/iceberg/pull/1433#discussion_r485080947
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import org.junit.Test;
+
+public class TestClientPool {
+
+ @Test(expected = RuntimeException.class)
+ public void testNewClientFailure() throws Exception {
+ try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
+ pool.run(Object::toString);
+ }
+ }
+
+ private static class MockClientPool extends ClientPool<Object, Exception> {
Review comment:
Can we call this something other than `MockClientPool`? It injects a failure, so I think a name like `FailNewClientPool` would be better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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] openinx commented on a change in pull request #1433: Fix the bug that clientPool#close will block forever when newClient encountered an exception.
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #1433:
URL: https://github.com/apache/iceberg/pull/1433#discussion_r485296334
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -114,8 +114,9 @@ private C get() throws InterruptedException {
if (!clients.isEmpty()) {
return clients.removeFirst();
} else if (currentSize < poolSize) {
+ C client = newClient();
currentSize += 1;
Review comment:
Yeah, that's 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.
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 merged pull request #1433: Fix the bug that clientPool#close will block forever when newClient encountered an exception.
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1433:
URL: https://github.com/apache/iceberg/pull/1433
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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] openinx commented on a change in pull request #1433: Fix the bug that clientPool#close will block forever when newClient encountered an exception.
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #1433:
URL: https://github.com/apache/iceberg/pull/1433#discussion_r485296530
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import org.junit.Test;
+
+public class TestClientPool {
+
+ @Test(expected = RuntimeException.class)
+ public void testNewClientFailure() throws Exception {
+ try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
+ pool.run(Object::toString);
+ }
+ }
+
+ private static class MockClientPool extends ClientPool<Object, Exception> {
Review comment:
That make sense, should I propose another pull request ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 pull request #1433: Fix the bug that clientPool#close will block forever when newClient encountered an exception.
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1433:
URL: https://github.com/apache/iceberg/pull/1433#issuecomment-689025110
Looks good to me. I had a couple comments on the tests (naming, use `AssertHelpers.assertThrows`), but those aren't blockers so I'll merge this. Good catch, @openinx!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org