You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nk1506 (via GitHub)" <gi...@apache.org> on 2024/04/03 11:36:38 UTC
[PR] Hive, JDBC: Add null check before matching the error message [iceberg]
nk1506 opened a new pull request, #10082:
URL: https://github.com/apache/iceberg/pull/10082
(no comment)
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1553049382
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -970,6 +976,51 @@ public void testCatalogWithCustomMetricsReporter() throws IOException {
Assertions.assertThat(CustomMetricsReporter.COUNTER.get()).isEqualTo(2);
}
+ @Test
+ public void testCommitExceptionWithoutMessage() {
+ TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl");
+ BaseTable table = (BaseTable) catalog.buildTable(tableIdent, SCHEMA).create();
+ JdbcTableOperations ops = (JdbcTableOperations) table.operations();
+ TableMetadata metadataV1 = ops.current();
+
+ table.updateSchema().addColumn("n", Types.IntegerType.get()).commit();
+ ops.refresh();
+
+ JdbcTableOperations spyOps = spy(ops);
+
+ try (MockedStatic<JdbcUtil> mockedStatic = Mockito.mockStatic(JdbcUtil.class)) {
+ mockedStatic
+ .when(() -> JdbcUtil.loadTable(any(), any(), any(), any()))
+ .thenThrow(new SQLException());
+ assertThatThrownBy(() -> spyOps.commit(ops.current(), metadataV1))
+ .isInstanceOf(UncheckedSQLException.class)
+ .hasMessageStartingWith("Unknown failure");
+ }
+ }
+
+ @Test
+ public void testCommitExceptionWithMessage() {
+ TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl");
+ BaseTable table = (BaseTable) catalog.buildTable(tableIdent, SCHEMA).create();
+ JdbcTableOperations ops = (JdbcTableOperations) table.operations();
+
+ TableMetadata metadataV1 = ops.current();
+
+ table.updateSchema().addColumn("n", Types.IntegerType.get()).commit();
+ ops.refresh();
+
+ JdbcTableOperations spyOps = spy(ops);
+
+ try (MockedStatic<JdbcUtil> mockedStatic = Mockito.mockStatic(JdbcUtil.class)) {
+ mockedStatic
+ .when(() -> JdbcUtil.loadTable(any(), any(), any(), any()))
+ .thenThrow(new SQLException("constraint failed"));
Review Comment:
shouldn't this have a null message?
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1553247096
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java:
##########
@@ -73,7 +73,8 @@ protected IMetaStoreClient newClient() {
} catch (MetaException e) {
throw new RuntimeMetaException(e, "Failed to connect to Hive Metastore");
} catch (Throwable t) {
- if (t.getMessage().contains("Another instance of Derby may have already booted")) {
+ if (t.getMessage() != null
Review Comment:
try to look at the existing tests in `TestHiveClientPool` and see how things could be mocked so that we get the right exception
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1551269821
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java:
##########
@@ -59,29 +59,19 @@ public HiveClientPool(int poolSize, Configuration conf) {
@Override
protected IMetaStoreClient newClient() {
try {
- try {
- return GET_CLIENT.invoke(
- hiveConf, (HiveMetaHookLoader) tbl -> null, HiveMetaStoreClient.class.getName());
- } catch (RuntimeException e) {
- // any MetaException would be wrapped into RuntimeException during reflection, so let's
- // double-check type here
- if (e.getCause() instanceof MetaException) {
Review Comment:
@nk1506 this isn't fixed. The logic in the try-catch blocks changed. The only thing that should be changing is adding a null check before calling `t.getMessage()`
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1551437288
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java:
##########
@@ -138,7 +138,7 @@ public void doCommit(TableMetadata base, TableMetadata metadata) {
throw new UncheckedSQLException(e, "Database warning");
} catch (SQLException e) {
// SQLite doesn't set SQLState or throw SQLIntegrityConstraintViolationException
- if (e.getMessage().contains("constraint failed")) {
+ if (e.getMessage() != null && e.getMessage().contains("constraint failed")) {
Review Comment:
please add tests that exercise each change here (similar to the test in https://github.com/apache/iceberg/pull/10069)
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1553429104
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -970,6 +975,47 @@ public void testCatalogWithCustomMetricsReporter() throws IOException {
Assertions.assertThat(CustomMetricsReporter.COUNTER.get()).isEqualTo(2);
}
+ @Test
+ public void testCommitExceptionWithoutMessage() {
+ TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl");
+ BaseTable table = (BaseTable) catalog.buildTable(tableIdent, SCHEMA).create();
+ JdbcTableOperations ops = (JdbcTableOperations) table.operations();
Review Comment:
no need for the casting here. See the simplified test version in my previous comment
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1553049382
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -970,6 +976,51 @@ public void testCatalogWithCustomMetricsReporter() throws IOException {
Assertions.assertThat(CustomMetricsReporter.COUNTER.get()).isEqualTo(2);
}
+ @Test
+ public void testCommitExceptionWithoutMessage() {
+ TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl");
+ BaseTable table = (BaseTable) catalog.buildTable(tableIdent, SCHEMA).create();
+ JdbcTableOperations ops = (JdbcTableOperations) table.operations();
+ TableMetadata metadataV1 = ops.current();
+
+ table.updateSchema().addColumn("n", Types.IntegerType.get()).commit();
+ ops.refresh();
+
+ JdbcTableOperations spyOps = spy(ops);
+
+ try (MockedStatic<JdbcUtil> mockedStatic = Mockito.mockStatic(JdbcUtil.class)) {
+ mockedStatic
+ .when(() -> JdbcUtil.loadTable(any(), any(), any(), any()))
+ .thenThrow(new SQLException());
+ assertThatThrownBy(() -> spyOps.commit(ops.current(), metadataV1))
+ .isInstanceOf(UncheckedSQLException.class)
+ .hasMessageStartingWith("Unknown failure");
+ }
+ }
+
+ @Test
+ public void testCommitExceptionWithMessage() {
+ TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl");
+ BaseTable table = (BaseTable) catalog.buildTable(tableIdent, SCHEMA).create();
+ JdbcTableOperations ops = (JdbcTableOperations) table.operations();
+
+ TableMetadata metadataV1 = ops.current();
+
+ table.updateSchema().addColumn("n", Types.IntegerType.get()).commit();
+ ops.refresh();
+
+ JdbcTableOperations spyOps = spy(ops);
+
+ try (MockedStatic<JdbcUtil> mockedStatic = Mockito.mockStatic(JdbcUtil.class)) {
+ mockedStatic
+ .when(() -> JdbcUtil.loadTable(any(), any(), any(), any()))
+ .thenThrow(new SQLException("constraint failed"));
Review Comment:
shouldn't this have a null message?
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1553083646
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -970,6 +976,51 @@ public void testCatalogWithCustomMetricsReporter() throws IOException {
Assertions.assertThat(CustomMetricsReporter.COUNTER.get()).isEqualTo(2);
}
+ @Test
+ public void testCommitExceptionWithoutMessage() {
+ TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl");
+ BaseTable table = (BaseTable) catalog.buildTable(tableIdent, SCHEMA).create();
+ JdbcTableOperations ops = (JdbcTableOperations) table.operations();
+ TableMetadata metadataV1 = ops.current();
+
+ table.updateSchema().addColumn("n", Types.IntegerType.get()).commit();
+ ops.refresh();
+
+ JdbcTableOperations spyOps = spy(ops);
+
+ try (MockedStatic<JdbcUtil> mockedStatic = Mockito.mockStatic(JdbcUtil.class)) {
+ mockedStatic
+ .when(() -> JdbcUtil.loadTable(any(), any(), any(), any()))
+ .thenThrow(new SQLException());
+ assertThatThrownBy(() -> spyOps.commit(ops.current(), metadataV1))
+ .isInstanceOf(UncheckedSQLException.class)
+ .hasMessageStartingWith("Unknown failure");
+ }
+ }
+
+ @Test
+ public void testCommitExceptionWithMessage() {
+ TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl");
+ BaseTable table = (BaseTable) catalog.buildTable(tableIdent, SCHEMA).create();
+ JdbcTableOperations ops = (JdbcTableOperations) table.operations();
+
+ TableMetadata metadataV1 = ops.current();
+
+ table.updateSchema().addColumn("n", Types.IntegerType.get()).commit();
+ ops.refresh();
+
+ JdbcTableOperations spyOps = spy(ops);
+
+ try (MockedStatic<JdbcUtil> mockedStatic = Mockito.mockStatic(JdbcUtil.class)) {
+ mockedStatic
+ .when(() -> JdbcUtil.loadTable(any(), any(), any(), any()))
+ .thenThrow(new SQLException("constraint failed"));
Review Comment:
Null message we are checking at other [testCommitExceptionWithoutMessage](https://github.com/apache/iceberg/pull/10082/files#diff-754dd3358fceeaa7214faa73eee5b8b0e71769f7d9a2b568a17191e42db9fce2R994).
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1553430668
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java:
##########
@@ -116,6 +119,52 @@ public void testGetTablesFailsForNonReconnectableException() throws Exception {
.hasMessage("Another meta exception");
}
+ @Test
+ public void testExceptionMessages() {
+ // Test Wrapped MetaException with a message
+ try (MockedStatic<MetaStoreUtils> mockedStatic = Mockito.mockStatic(MetaStoreUtils.class)) {
Review Comment:
Couldn't find a way to mock calling method
```
return GET_CLIENT.invoke(
hiveConf, (HiveMetaHookLoader) tbl -> null, HiveMetaStoreClient.class.getName());
```
and throw Exception. Had to trace and mock the class which is throwing error.
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1551178019
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java:
##########
@@ -59,29 +59,19 @@ public HiveClientPool(int poolSize, Configuration conf) {
@Override
protected IMetaStoreClient newClient() {
try {
- try {
- return GET_CLIENT.invoke(
- hiveConf, (HiveMetaHookLoader) tbl -> null, HiveMetaStoreClient.class.getName());
- } catch (RuntimeException e) {
- // any MetaException would be wrapped into RuntimeException during reflection, so let's
- // double-check type here
- if (e.getCause() instanceof MetaException) {
Review Comment:
Fixed it.
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1550973142
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java:
##########
@@ -59,29 +59,19 @@ public HiveClientPool(int poolSize, Configuration conf) {
@Override
protected IMetaStoreClient newClient() {
try {
- try {
- return GET_CLIENT.invoke(
- hiveConf, (HiveMetaHookLoader) tbl -> null, HiveMetaStoreClient.class.getName());
- } catch (RuntimeException e) {
- // any MetaException would be wrapped into RuntimeException during reflection, so let's
- // double-check type here
- if (e.getCause() instanceof MetaException) {
Review Comment:
@nk1506 why is this being removed?
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1551350882
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java:
##########
@@ -59,29 +59,19 @@ public HiveClientPool(int poolSize, Configuration conf) {
@Override
protected IMetaStoreClient newClient() {
try {
- try {
- return GET_CLIENT.invoke(
- hiveConf, (HiveMetaHookLoader) tbl -> null, HiveMetaStoreClient.class.getName());
- } catch (RuntimeException e) {
- // any MetaException would be wrapped into RuntimeException during reflection, so let's
- // double-check type here
- if (e.getCause() instanceof MetaException) {
Review Comment:
the other try-catch block looks redundant . We were wrapping into [MetaException](https://github.com/apache/iceberg/blob/main/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java#L69) and throwing and again [wrapping](https://github.com/apache/iceberg/blob/main/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java#L74) into RuntimeMetaException .
Should I revert and add only null check ?
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1553435328
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java:
##########
@@ -116,6 +119,52 @@ public void testGetTablesFailsForNonReconnectableException() throws Exception {
.hasMessage("Another meta exception");
}
+ @Test
+ public void testExceptionMessages() {
+ // Test Wrapped MetaException with a message
Review Comment:
I don't think those comments are helpful. I would just remove all of those
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1552720185
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java:
##########
@@ -73,7 +73,8 @@ protected IMetaStoreClient newClient() {
} catch (MetaException e) {
throw new RuntimeMetaException(e, "Failed to connect to Hive Metastore");
} catch (Throwable t) {
- if (t.getMessage().contains("Another instance of Derby may have already booted")) {
+ if (t.getMessage() != null
Review Comment:
Having trouble to add tests for this. Since `GET_CLIENT` is static.
@nastra Any pointers will help.
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1551267638
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java:
##########
@@ -59,29 +59,19 @@ public HiveClientPool(int poolSize, Configuration conf) {
@Override
protected IMetaStoreClient newClient() {
try {
- try {
- return GET_CLIENT.invoke(
- hiveConf, (HiveMetaHookLoader) tbl -> null, HiveMetaStoreClient.class.getName());
- } catch (RuntimeException e) {
- // any MetaException would be wrapped into RuntimeException during reflection, so let's
- // double-check type here
- if (e.getCause() instanceof MetaException) {
Review Comment:
Let me lookout/add some tests which can catch these errors.
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1551398196
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java:
##########
@@ -59,29 +59,19 @@ public HiveClientPool(int poolSize, Configuration conf) {
@Override
protected IMetaStoreClient newClient() {
try {
- try {
- return GET_CLIENT.invoke(
- hiveConf, (HiveMetaHookLoader) tbl -> null, HiveMetaStoreClient.class.getName());
- } catch (RuntimeException e) {
- // any MetaException would be wrapped into RuntimeException during reflection, so let's
- // double-check type here
- if (e.getCause() instanceof MetaException) {
Review Comment:
yes, please revert. The scope within the PR should reflect what's being proposed in the commit msg
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10082:
URL: https://github.com/apache/iceberg/pull/10082#discussion_r1553236192
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -970,6 +976,51 @@ public void testCatalogWithCustomMetricsReporter() throws IOException {
Assertions.assertThat(CustomMetricsReporter.COUNTER.get()).isEqualTo(2);
}
+ @Test
+ public void testCommitExceptionWithoutMessage() {
Review Comment:
can be simplified to
```
@Test
public void testCommitExceptionWithoutMessage() {
TableIdentifier tableIdent = TableIdentifier.of("db", "tbl");
BaseTable table = (BaseTable) catalog.buildTable(tableIdent, SCHEMA).create();
TableOperations ops = table.operations();
TableMetadata metadataV1 = ops.current();
table.updateSchema().addColumn("n", Types.IntegerType.get()).commit();
ops.refresh();
try (MockedStatic<JdbcUtil> mockedStatic = Mockito.mockStatic(JdbcUtil.class)) {
mockedStatic
.when(() -> JdbcUtil.loadTable(any(), any(), any(), any()))
.thenThrow(new SQLException());
assertThatThrownBy(() -> ops.commit(ops.current(), metadataV1))
.isInstanceOf(UncheckedSQLException.class)
.hasMessageStartingWith("Unknown failure");
}
}
```
please also update the other tests accordingly. The spying on `ops` isn't necessary here, because you're throwing the exception when the table/view is being loaded (which is different from https://github.com/apache/iceberg/pull/10069)
--
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
Re: [PR] Hive, JDBC: Add null check before matching the error message [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10082:
URL: https://github.com/apache/iceberg/pull/10082
--
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