You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2021/10/06 23:02:48 UTC
[GitHub] [phoenix] ankitjain64 opened a new pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
ankitjain64 opened a new pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328
JIRA: https://issues.apache.org/jira/browse/PHOENIX-6548
In our pipelines, we saw that after the ClusterConnection is received from [ConnectionFactory](https://github.com/apache/phoenix/blob/4.x/phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java#L362) the region server crashes and the underlying connection is closed which causes IllegalArgumentException from hbase in [getTable](https://github.com/apache/phoenix/blob/4.x/phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java#L317) and DoNotRetryIOException is sent back to client.
Instead, we want the client exception to be a normal retriable IOException, because if they try again after the region comes up again somewhere else, the Scan will likely succeed.
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gokceni commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723747526
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -308,13 +308,22 @@ public static HTableFactory getDelegateHTableFactory(CoprocessorEnvironment env,
this.connectionType = connectionType;
}
- private ClusterConnection getConnection() throws IOException {
+ public ClusterConnection getConnection() throws IOException {
return ConnectionFactory.getConnection(connectionType, conf, server);
}
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename) throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary());
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary());
Review comment:
Should you do a null check here? Can connection be null?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -308,13 +308,22 @@ public static HTableFactory getDelegateHTableFactory(CoprocessorEnvironment env,
this.connectionType = connectionType;
}
- private ClusterConnection getConnection() throws IOException {
+ public ClusterConnection getConnection() throws IOException {
return ConnectionFactory.getConnection(connectionType, conf, server);
}
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename) throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary());
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary());
Review comment:
Should you do a null check here? Can connection be null? If yes then IllegalArgumentException will not be thrown
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
ditto
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ankitjain64 commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r724584127
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
Done in latest commit.
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ankitjain64 commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r724465972
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
@tkhurana I also thought of doing exactly this while making the above change. But while checking the underlying hbase implementations, when we pass `pool = null` this [getDefaultExecutor](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java#L213) is called [here](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java#L320) to create the ThreadPool, but in the scenario when no pool object is passed like in our case on [line 320](https://github.com/apache/phoenix/pull/1328/files#diff-28b6df839a384471211d620c7845ea2a40c8ef67e35966a07122bb4f8938086dR320) the underlying ThreadPool creation is different [Link](https://github.com/apache/phoenix/pull/1328/files#diff-28b6df839a384471211d620c7845ea2a40c8ef67e35966a07122bb4f8938086dR320) and [getBatchPool](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/C
onnectionManager.java#L827) is called. That's why I did merge those two function calls.
Please let me know if I am missing anything and you still think we should make the above refactor. Thanks
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ankitjain64 commented on pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#issuecomment-939179515
> Thank you @ankitjain64 for fixing this. Can you please also create PR against master branch?
Created here: https://github.com/apache/phoenix/pull/1329
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#issuecomment-938505575
Thank you @ankitjain64 for fixing this. Can you please also create PR against master branch?
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r724518983
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
Won't something like this work
```
commonAPI(tablename, pool) {
if (pool == null)
connection.getTable(tablename);
else
connection.getTable(tablename, pool)
}
```
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gokceni commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723747526
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -308,13 +308,22 @@ public static HTableFactory getDelegateHTableFactory(CoprocessorEnvironment env,
this.connectionType = connectionType;
}
- private ClusterConnection getConnection() throws IOException {
+ public ClusterConnection getConnection() throws IOException {
return ConnectionFactory.getConnection(connectionType, conf, server);
}
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename) throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary());
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary());
Review comment:
Should you do a null check here? Can connection be null?
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723753018
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/util/ServerUtilTest.java
##########
@@ -0,0 +1,66 @@
+/**
+ * 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.phoenix.util;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.client.ClusterConnection;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr;
+import org.apache.phoenix.query.HBaseFactoryProvider;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.concurrent.ExecutorService;
+
+public class ServerUtilTest {
+
+ @Test
+ public void testCoprocessorHConnectionGetTableWithClosedConnection() throws Exception {
+ Configuration conf = HBaseFactoryProvider.getConfigurationFactory().getConfiguration();
+ HRegionServer server = Mockito.mock(HRegionServer.class);
+ ServerUtil.ConnectionType connectionType = ServerUtil.ConnectionType.INDEX_WRITER_CONNECTION;
+
+ // Mock ClusterConnection object to throw IllegalArgumentException.
+ ClusterConnection connection = Mockito.mock(ClusterConnection.class);
+ Mockito.doThrow(new IllegalArgumentException()).when(connection).getTable(
+ Mockito.<byte[]>any());
+ Mockito.doThrow(new IllegalArgumentException()).when(connection).getTable(
+ Mockito.<byte[]>any(), Mockito.<ExecutorService>any());
+ Mockito.doReturn(true).when(connection).isClosed();
+
+ // Spy CoprocessorHConnectionTableFactory
+ ServerUtil.CoprocessorHConnectionTableFactory coprocHTableFactory = new ServerUtil.
+ CoprocessorHConnectionTableFactory(conf, server, connectionType);
+ ServerUtil.CoprocessorHConnectionTableFactory spyedObj = Mockito.spy(coprocHTableFactory);
+ Mockito.doReturn(connection).when(spyedObj).getConnection();
+
+ try {
+ spyedObj.getTable(new ImmutableBytesPtr(Bytes.toBytes("test_table")));
+ Assert.fail("IOException exception expected as connection was closed");
+ }catch (IOException e1) {
Review comment:
Would be good to have an initial catch block that catches DoNotRetryIOException and Assert fails (because DNRIE is a subclass of IOException so the current test would pass even if it was thrown)
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gokceni commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723747699
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
ditto
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ankitjain64 commented on pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#issuecomment-937307168
@gokceni @gjacoby126 @tkhurana. Please have a look when you get a chance. Thanks.
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ankitjain64 commented on pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#issuecomment-937307168
@gokceni @gjacoby126 @tkhurana. Please have a look when you get a chance. Thanks.
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 merged pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
gjacoby126 merged pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ankitjain64 commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723751701
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -308,13 +308,22 @@ public static HTableFactory getDelegateHTableFactory(CoprocessorEnvironment env,
this.connectionType = connectionType;
}
- private ClusterConnection getConnection() throws IOException {
+ public ClusterConnection getConnection() throws IOException {
return ConnectionFactory.getConnection(connectionType, conf, server);
}
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename) throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary());
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary());
Review comment:
It should not be since in ConnectionFactory we have that check-in place. [link](https://github.com/apache/phoenix/blob/4.x/phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java#L362).
Also on hbase side we throw IAE in case connection is null [here](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java#L307)
Let me know if you think otherwise.
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ankitjain64 commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723751701
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -308,13 +308,22 @@ public static HTableFactory getDelegateHTableFactory(CoprocessorEnvironment env,
this.connectionType = connectionType;
}
- private ClusterConnection getConnection() throws IOException {
+ public ClusterConnection getConnection() throws IOException {
return ConnectionFactory.getConnection(connectionType, conf, server);
}
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename) throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary());
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary());
Review comment:
It should not be since in ConnectionFactory we have that check-in place. [link](https://github.com/apache/phoenix/blob/4.x/phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java#L362).
Also on hbase side we throw IAE in case connection is null [here](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java#L307)
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723753018
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/util/ServerUtilTest.java
##########
@@ -0,0 +1,66 @@
+/**
+ * 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.phoenix.util;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.client.ClusterConnection;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr;
+import org.apache.phoenix.query.HBaseFactoryProvider;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.concurrent.ExecutorService;
+
+public class ServerUtilTest {
+
+ @Test
+ public void testCoprocessorHConnectionGetTableWithClosedConnection() throws Exception {
+ Configuration conf = HBaseFactoryProvider.getConfigurationFactory().getConfiguration();
+ HRegionServer server = Mockito.mock(HRegionServer.class);
+ ServerUtil.ConnectionType connectionType = ServerUtil.ConnectionType.INDEX_WRITER_CONNECTION;
+
+ // Mock ClusterConnection object to throw IllegalArgumentException.
+ ClusterConnection connection = Mockito.mock(ClusterConnection.class);
+ Mockito.doThrow(new IllegalArgumentException()).when(connection).getTable(
+ Mockito.<byte[]>any());
+ Mockito.doThrow(new IllegalArgumentException()).when(connection).getTable(
+ Mockito.<byte[]>any(), Mockito.<ExecutorService>any());
+ Mockito.doReturn(true).when(connection).isClosed();
+
+ // Spy CoprocessorHConnectionTableFactory
+ ServerUtil.CoprocessorHConnectionTableFactory coprocHTableFactory = new ServerUtil.
+ CoprocessorHConnectionTableFactory(conf, server, connectionType);
+ ServerUtil.CoprocessorHConnectionTableFactory spyedObj = Mockito.spy(coprocHTableFactory);
+ Mockito.doReturn(connection).when(spyedObj).getConnection();
+
+ try {
+ spyedObj.getTable(new ImmutableBytesPtr(Bytes.toBytes("test_table")));
+ Assert.fail("IOException exception expected as connection was closed");
+ }catch (IOException e1) {
Review comment:
Would be good to have an initial catch block that catches DoNotRetryIOException and Assert fails (because DNRIE is a subclass of IOException so the current test would pass even if it was thrown)
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gokceni commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723747526
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -308,13 +308,22 @@ public static HTableFactory getDelegateHTableFactory(CoprocessorEnvironment env,
this.connectionType = connectionType;
}
- private ClusterConnection getConnection() throws IOException {
+ public ClusterConnection getConnection() throws IOException {
return ConnectionFactory.getConnection(connectionType, conf, server);
}
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename) throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary());
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary());
Review comment:
Should you do a null check here? Can connection be null? If yes then IllegalArgumentException will not be thrown
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ankitjain64 commented on pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#issuecomment-939147240
> @ankitjain64 - looks like there was a test failure of PhoenixTableLevelMetricsIT when running against your patch. I just ran that test without your patch locally and it passed. Could be a flapper, but could you please check if you can reproduce the failure?
It passes locally for me too.
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ankitjain64 commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723751701
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -308,13 +308,22 @@ public static HTableFactory getDelegateHTableFactory(CoprocessorEnvironment env,
this.connectionType = connectionType;
}
- private ClusterConnection getConnection() throws IOException {
+ public ClusterConnection getConnection() throws IOException {
return ConnectionFactory.getConnection(connectionType, conf, server);
}
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename) throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary());
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary());
Review comment:
It should not be since in ConnectionFactory we have that check-in place. [link](https://github.com/apache/phoenix/blob/4.x/phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java#L362).
Also on hbase side we throw IAE in case connection is null [here](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java#L307)
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -308,13 +308,22 @@ public static HTableFactory getDelegateHTableFactory(CoprocessorEnvironment env,
this.connectionType = connectionType;
}
- private ClusterConnection getConnection() throws IOException {
+ public ClusterConnection getConnection() throws IOException {
return ConnectionFactory.getConnection(connectionType, conf, server);
}
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename) throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary());
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary());
Review comment:
It should not be since in ConnectionFactory we have that check-in place. [link](https://github.com/apache/phoenix/blob/4.x/phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java#L362).
Also on hbase side we throw IAE in case connection is null [here](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java#L307)
Let me know if you think otherwise.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
@tkhurana I also thought of doing exactly this while making the above change. But while checking the underlying hbase implementations, when we pass `pool = null` this [getDefaultExecutor](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java#L213) is called [here](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java#L320) to create the ThreadPool, but in the scenario when no pool object is passed like in our case on [line 320](https://github.com/apache/phoenix/pull/1328/files#diff-28b6df839a384471211d620c7845ea2a40c8ef67e35966a07122bb4f8938086dR320) the underlying ThreadPool creation is different [Link](https://github.com/apache/phoenix/pull/1328/files#diff-28b6df839a384471211d620c7845ea2a40c8ef67e35966a07122bb4f8938086dR320) and [getBatchPool](https://github.com/apache/hbase/blob/branch-1.4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/C
onnectionManager.java#L827) is called. That's why I did merge those two function calls.
Please let me know if I am missing anything and you still think we should make the above refactor. Thanks
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
Done in latest commit.
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723883970
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
@ankitjain64 can we refactor the two getTable functions to reduce duplicate code. We can pass a null for the pool object and depending upon the value use the corresponding connection.getTable() method.
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#discussion_r723883970
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
@ankitjain64 can we refactor the two getTable functions to reduce duplicate code. We can pass a null for the pool object and depending upon the value use the corresponding connection.getTable() method.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ServerUtil.java
##########
@@ -328,7 +337,16 @@ public void shutdown() {
@Override
public HTableInterface getTable(ImmutableBytesPtr tablename, ExecutorService pool)
throws IOException {
- return getConnection().getTable(tablename.copyBytesIfNecessary(), pool);
+ ClusterConnection connection = null;
+ try {
+ connection = getConnection();
+ return connection.getTable(tablename.copyBytesIfNecessary(), pool);
+ }catch (IllegalArgumentException e) {
Review comment:
Won't something like this work
```
commonAPI(tablename, pool) {
if (pool == null)
connection.getTable(tablename);
else
connection.getTable(tablename, pool)
}
```
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on pull request #1328: PHOENIX-6548: Throw IOException instead of IllegalArgumentException when RS crashes during index rebuilds.
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on pull request #1328:
URL: https://github.com/apache/phoenix/pull/1328#issuecomment-938749417
@ankitjain64 - looks like there was a test failure of PhoenixTableLevelMetricsIT when running against your patch. I just ran that test without your patch locally and it passed. Could be a flapper, but could you please check if you can reproduce the failure?
--
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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org