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/04/17 12:10:32 UTC
[GitHub] [iceberg] ismailsimsek opened a new pull request #2491: Moving ClientPool and ClientPoolImpl to core
ismailsimsek opened a new pull request #2491:
URL: https://github.com/apache/iceberg/pull/2491
Moving client pool to core module so it could be used by other modules ex: jdbc catalog https://github.com/apache/iceberg/pull/1870
--
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 #2491: Moving ClientPool and ClientPoolImpl to core
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2491:
URL: https://github.com/apache/iceberg/pull/2491#discussion_r615323229
##########
File path: core/src/main/java/org/apache/iceberg/ClientPoolImpl.java
##########
@@ -138,8 +137,7 @@ private void release(C client) {
}
}
- @VisibleForTesting
- int poolSize() {
+ public int poolSize() {
Review comment:
Why make this public?
--
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 #2491: Moving ClientPool and ClientPoolImpl to core
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2491:
URL: https://github.com/apache/iceberg/pull/2491#discussion_r615323400
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java
##########
@@ -23,9 +23,23 @@
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestHiveClientPool {
Review comment:
I think this is correct.
--
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] pvary commented on a change in pull request #2491: Moving ClientPool and ClientPoolImpl to core
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2491:
URL: https://github.com/apache/iceberg/pull/2491#discussion_r615782225
##########
File path: core/src/main/java/org/apache/iceberg/ClientPoolImpl.java
##########
@@ -138,8 +137,7 @@ private void release(C client) {
}
}
- @VisibleForTesting
- int poolSize() {
+ public int poolSize() {
Review comment:
@ismailsimsek: Is this public because previously the package was `org.apache.iceberg.hive` and now the files are moved to `org.apache.iceberg`, so package private access modifier is not enough anymore?
--
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] ismailsimsek commented on a change in pull request #2491: Moving ClientPool and ClientPoolImpl to core
Posted by GitBox <gi...@apache.org>.
ismailsimsek commented on a change in pull request #2491:
URL: https://github.com/apache/iceberg/pull/2491#discussion_r615802905
##########
File path: core/src/main/java/org/apache/iceberg/ClientPoolImpl.java
##########
@@ -138,8 +137,7 @@ private void release(C client) {
}
}
- @VisibleForTesting
- int poolSize() {
+ public int poolSize() {
Review comment:
@pvary correct, its not working when the test is in another module.
--
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 #2491: Moving ClientPool and ClientPoolImpl to core
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2491:
URL: https://github.com/apache/iceberg/pull/2491
--
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] ismailsimsek commented on a change in pull request #2491: Moving ClientPool and ClientPoolImpl to core
Posted by GitBox <gi...@apache.org>.
ismailsimsek commented on a change in pull request #2491:
URL: https://github.com/apache/iceberg/pull/2491#discussion_r615248034
##########
File path: core/src/main/java/org/apache/iceberg/ClientPoolImpl.java
##########
@@ -138,8 +137,7 @@ private void release(C client) {
}
}
- @VisibleForTesting
- int poolSize() {
+ public int poolSize() {
Review comment:
had to change it to public to get hive test working, seems like VisibleForTesting is not possible across different modules
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java
##########
@@ -23,9 +23,23 @@
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestHiveClientPool {
Review comment:
Moved TestClientPoolImpl.java tests to this class since it was using Hive libraries in the tests, merged tests in this class.
--
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