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/01/06 01:23:09 UTC
[GitHub] [phoenix] wangchao316 opened a new pull request #1064: PHOENIX-6050 Set properties is invalid in client
wangchao316 opened a new pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064
----------------------------------------------------------------
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
[GitHub] [phoenix] wangchao316 commented on pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
wangchao316 commented on pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#issuecomment-755262426
> I agree @wangchao316 that tuning poolsize is crucial for performance optimization.
> It is just my opinion that the tuning should be done on the cluster size, in hbase-site.xml, rather than on the client side.
yes, I feel we provide this parameter, should be altered in effect performance place.
phoenix deploy in phoenix-server mode and no phoenix-server mode.
1. if be no phoenix-server mode, client will be compiler, optimization，scan to hbase-regionserver side. so client will use this parameter in ParallelIterator.java， which effect scan.
2. if be phoenix-server mode, we will alter parameter in phoenix-server .
----------------------------------------------------------------
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
[GitHub] [phoenix] stoty commented on pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#issuecomment-755200344
Also see my objection from the ticket that this will cause discrepancy between the poolsize used by the client, and by the Index rebuilding initiated by MetaDataObserver.
----------------------------------------------------------------
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
[GitHub] [phoenix] yanxinyi commented on a change in pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
yanxinyi commented on a change in pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#discussion_r552451067
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixDriver.java
##########
@@ -182,7 +196,7 @@ public void onRemoval(RemovalNotification<ConnectionInfo, ConnectionQueryService
@Override
- public QueryServices getQueryServices() throws SQLException {
+ public QueryServices getQueryServices(final Properties info) throws SQLException {
Review comment:
Adding extra argument won’t override the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] wangchao316 commented on pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
wangchao316 commented on pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#issuecomment-755231212
and I find from 4.x to 5.x, reduce performance in some changed, which consumer use in our product environment by upgrading. I will always deal this question later.
----------------------------------------------------------------
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
[GitHub] [phoenix] yanxinyi commented on a change in pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
yanxinyi commented on a change in pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#discussion_r552450031
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixDriver.java
##########
@@ -148,21 +160,23 @@ public PhoenixDriver() { // for Squirrel
}
private Cache<ConnectionInfo, ConnectionQueryServices> initializeConnectionCache() {
- Configuration config = HBaseFactoryProvider.getConfigurationFactory().getConfiguration();
+ Configuration config = HBaseFactoryProvider.getConfigurationFactory()
+ .getConfiguration();
int maxCacheDuration = config.getInt(QueryServices.CLIENT_CONNECTION_CACHE_MAX_DURATION_MILLISECONDS,
QueryServicesOptions.DEFAULT_CLIENT_CONNECTION_CACHE_MAX_DURATION);
RemovalListener<ConnectionInfo, ConnectionQueryServices> cacheRemovalListener =
new RemovalListener<ConnectionInfo, ConnectionQueryServices>() {
@Override
public void onRemoval(RemovalNotification<ConnectionInfo, ConnectionQueryServices> notification) {
String connInfoIdentifier = notification.getKey().toString();
- LOGGER.debug("Expiring " + connInfoIdentifier + " because of "
- + notification.getCause().name());
+ if (LOGGER.isDebugEnabled()) {
Review comment:
Do we need if check 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#issuecomment-755199276
I am generally not convinced that overriding poolsize from the client side is a good idea.
It is very easy to overload the cluster by specifying a too large poolsize.
IMO it's better to leave this knob in the cluster administrator's hand.
----------------------------------------------------------------
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
[GitHub] [phoenix] wangchao316 commented on a change in pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
wangchao316 commented on a change in pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#discussion_r552454415
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixDriver.java
##########
@@ -182,7 +196,7 @@ public void onRemoval(RemovalNotification<ConnectionInfo, ConnectionQueryService
@Override
- public QueryServices getQueryServices() throws SQLException {
+ public QueryServices getQueryServices(final Properties info) throws SQLException {
Review comment:
> Adding extra argument won’t override the method
I find this getQueryServices only use in inner the project， only test and PhoenixDriver use. client use by PhoenixDriver, do not oveeride this interface.
----------------------------------------------------------------
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
[GitHub] [phoenix] wangchao316 commented on a change in pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
wangchao316 commented on a change in pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#discussion_r552451847
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixDriver.java
##########
@@ -148,21 +160,23 @@ public PhoenixDriver() { // for Squirrel
}
private Cache<ConnectionInfo, ConnectionQueryServices> initializeConnectionCache() {
- Configuration config = HBaseFactoryProvider.getConfigurationFactory().getConfiguration();
+ Configuration config = HBaseFactoryProvider.getConfigurationFactory()
+ .getConfiguration();
int maxCacheDuration = config.getInt(QueryServices.CLIENT_CONNECTION_CACHE_MAX_DURATION_MILLISECONDS,
QueryServicesOptions.DEFAULT_CLIENT_CONNECTION_CACHE_MAX_DURATION);
RemovalListener<ConnectionInfo, ConnectionQueryServices> cacheRemovalListener =
new RemovalListener<ConnectionInfo, ConnectionQueryServices>() {
@Override
public void onRemoval(RemovalNotification<ConnectionInfo, ConnectionQueryServices> notification) {
String connInfoIdentifier = notification.getKey().toString();
- LOGGER.debug("Expiring " + connInfoIdentifier + " because of "
- + notification.getCause().name());
+ if (LOGGER.isDebugEnabled()) {
Review comment:
thanks review, pheonix checkstyle do have, but logger use by this in industry.
----------------------------------------------------------------
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
[GitHub] [phoenix] yanxinyi commented on a change in pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
yanxinyi commented on a change in pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#discussion_r552453450
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
##########
@@ -91,8 +89,15 @@
protected ReadOnlyProps getDefaultProps() {
return DEFAULT_PROPS;
}
-
- abstract public QueryServices getQueryServices() throws SQLException;
+
+ /**
+ * get query services
+ * @param info properties info
+ * @return query services
+ * @throws SQLException if failed to get query service.
+ */
+ public abstract QueryServices getQueryServices(Properties info)
Review comment:
I see. You made an interface changes. I would suggest keep the original method with depreciation tag and remove it after two release.
----------------------------------------------------------------
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
[GitHub] [phoenix] wangchao316 commented on pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
wangchao316 commented on pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#issuecomment-755225514
> Also see my objection from the ticket that this will cause discrepancy between the poolsize used by the client, and by the Index rebuilding initiated by MetaDataObserver.
Thanks @stoty , I deal this ，have a big reason.
when have not use phoenix-server, compiler and hbase scan operate in client-side, and scan by poolsize in parallelIterator. I test in our prodect environment, this performance improvement more.
----------------------------------------------------------------
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
[GitHub] [phoenix] wangchao316 commented on pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
wangchao316 commented on pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#issuecomment-755163206
@virajjasani @gjacoby126 could you please review this ?
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#issuecomment-755249585
I agree @wangchao316 that tuning poolsize is crucial for performance optimization.
It is just my opinion that the tuning should be done on the cluster size, in hbase-site.xml, rather than on the client side.
----------------------------------------------------------------
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
[GitHub] [phoenix] wangchao316 commented on a change in pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
wangchao316 commented on a change in pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#discussion_r552455806
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
##########
@@ -91,8 +89,15 @@
protected ReadOnlyProps getDefaultProps() {
return DEFAULT_PROPS;
}
-
- abstract public QueryServices getQueryServices() throws SQLException;
+
+ /**
+ * get query services
+ * @param info properties info
+ * @return query services
+ * @throws SQLException if failed to get query service.
+ */
+ public abstract QueryServices getQueryServices(Properties info)
Review comment:
> I see. You made an interface changes. I would suggest keep the original method with depreciation tag and remove it after two release.
thanks , this is a good idea, I will do..
----------------------------------------------------------------
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
[GitHub] [phoenix] yanxinyi commented on a change in pull request #1064: PHOENIX-6050 Set properties is invalid
Posted by GitBox <gi...@apache.org>.
yanxinyi commented on a change in pull request #1064:
URL: https://github.com/apache/phoenix/pull/1064#discussion_r552451886
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java
##########
@@ -184,22 +184,22 @@
public abstract class BaseTest {
public static final String DRIVER_CLASS_NAME_ATTRIB = "phoenix.driver.class.name";
private static final double ZERO = 1e-9;
- private static final Map<String,String> tableDDLMap;
+ private static final Map<String, String> tableDDLMap;
private static final Logger LOGGER = LoggerFactory.getLogger(BaseTest.class);
@ClassRule
public static TemporaryFolder tmpFolder = new TemporaryFolder();
- private static final int dropTableTimeout = 300; // 5 mins should be long enough.
- private static final ThreadFactory factory = new ThreadFactoryBuilder().setDaemon(true)
+ private static final int DROP_TABLE_TIMEOUT = 300; // 5 mins should be long enough.
Review comment:
Nice coding style changing!
----------------------------------------------------------------
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