Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-03-07 Thread via GitHub 


shahrs87 commented on PR #1839:
URL: https://github.com/apache/phoenix/pull/1839#issuecomment-1984168480

   Change the title of the PR and commit to "PHOENIX-7243 : Add connectionType 
property to ConnectionInfo"
   There is 1 checkstyle warning that needs to be addressed.


-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-03-05 Thread via GitHub 


shahrs87 commented on PR #1838:
URL: https://github.com/apache/phoenix/pull/1838#issuecomment-1979314957

   Thank you @stoty for your comment.
   Mostly this is a problem on the test side where PhoenixTestDriver is caching 
the server and non server connections.
   Looking at the code, MDEI always creates server connections but there is 
nothing enforcing in the code so there is a possibility that someone might 
create non server connection also.
   


-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-03-05 Thread via GitHub 


stoty commented on PR #1838:
URL: https://github.com/apache/phoenix/pull/1838#issuecomment-1978334436

   I am not against this change, I just want to understand the problem better.
   
   Is there any reason ever not to use a server connection (or at least use the 
server connection methods to create it) on the server side ?
   Maybe it would be better to make sure that CQSI always uses server 
connections on the server side ?)
   Would that work ?


-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-03-04 Thread via GitHub 


palashc commented on code in PR #1839:
URL: https://github.com/apache/phoenix/pull/1839#discussion_r1511993390


##
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ConnectionInfo.java:
##
@@ -36,6 +36,7 @@
 import org.apache.phoenix.query.HBaseFactoryProvider;
 import org.apache.phoenix.query.QueryServices;
 import org.apache.phoenix.util.PhoenixRuntime;
+import org.apache.phoenix.util.QueryUtil;

Review Comment:
   Needed here - 
https://github.com/apache/phoenix/pull/1839/files#diff-1bb66a6bc989c1d63acbf10ef6f3f05dd9d86b6ec065f437d0b9332666b1f688R391



-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-03-04 Thread via GitHub 


shahrs87 commented on code in PR #1839:
URL: https://github.com/apache/phoenix/pull/1839#discussion_r1508286177


##
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/AbstractRPCConnectionInfo.java:
##
@@ -45,8 +45,8 @@ public String getBoostrapServers() {
 }
 
 protected AbstractRPCConnectionInfo(boolean isConnectionless, String 
principal, String keytab,
-User user, String haGroup) {
-super(isConnectionless, principal, keytab, user, haGroup);
+User user, String haGroup, Boolean isServerConnection) {

Review Comment:
   can we change `Boolean` --> `boolean` ?



-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-03-04 Thread via GitHub 


shahrs87 commented on code in PR #1839:
URL: https://github.com/apache/phoenix/pull/1839#discussion_r1508286177


##
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/AbstractRPCConnectionInfo.java:
##
@@ -45,8 +45,8 @@ public String getBoostrapServers() {
 }
 
 protected AbstractRPCConnectionInfo(boolean isConnectionless, String 
principal, String keytab,
-User user, String haGroup) {
-super(isConnectionless, principal, keytab, user, haGroup);
+User user, String haGroup, Boolean isServerConnection) {

Review Comment:
   can we change `Boolean` --> `boolean` ?



##
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ConnectionInfo.java:
##
@@ -36,6 +36,7 @@
 import org.apache.phoenix.query.HBaseFactoryProvider;
 import org.apache.phoenix.query.QueryServices;
 import org.apache.phoenix.util.PhoenixRuntime;
+import org.apache.phoenix.util.QueryUtil;

Review Comment:
   Is this needed?



-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-03-01 Thread via GitHub 


palashc commented on PR #1838:
URL: https://github.com/apache/phoenix/pull/1838#issuecomment-1973876443

   @stoty This came up in 
[PHOENIX-6883](https://issues.apache.org/jira/browse/PHOENIX-6883) where we 
want to invalidate the metadata cache on a region server and only a server 
connection should be allowed to do so. Since we enforced that condition, some 
tests which tried to perform invalidation on a non-server connection failed. 


-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-03-01 Thread via GitHub 


stoty commented on PR #1838:
URL: https://github.com/apache/phoenix/pull/1838#issuecomment-1973246638

   Server connections are always opened on the server, and they are only 
special in that they are cached and cannot be closed.
   
   How would creating a new server connection help if we already have a similar 
non-server connection ?


-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-02-28 Thread via GitHub 


palashc closed pull request #1838: PHOENIX-7243 : Add isServerConnection 
property to ConnectionInfo
URL: https://github.com/apache/phoenix/pull/1838


-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-02-28 Thread via GitHub 


palashc commented on PR #1838:
URL: https://github.com/apache/phoenix/pull/1838#issuecomment-1970198673

   Opened https://github.com/apache/phoenix/pull/1839 for master branch 
@shahrs87 


-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-02-28 Thread via GitHub 


palashc opened a new pull request, #1839:
URL: https://github.com/apache/phoenix/pull/1839

   (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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-02-28 Thread via GitHub 


palashc commented on code in PR #1838:
URL: https://github.com/apache/phoenix/pull/1838#discussion_r1506758361


##
phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixTestDriver.java:
##
@@ -92,7 +93,8 @@ public synchronized Connection connect(String url, Properties 
info) throws SQLEx
 public synchronized ConnectionQueryServices 
getConnectionQueryServices(String url, Properties infoIn) throws SQLException {
 checkClosed();
 final Properties info = PropertiesUtil.deepCopy(infoIn);
-ConnectionInfo connInfo = ConnectionInfo.create(url, null, info);
+boolean isServerConnection = 
Boolean.valueOf(info.getProperty(QueryUtil.IS_SERVER_CONNECTION));
+ConnectionInfo connInfo = ConnectionInfo.create(url, null, info, 
isServerConnection);

Review Comment:
   Pushed the refactor, no changes in Drivers needed. 



##
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ConnectionInfo.java:
##
@@ -114,6 +115,15 @@ public static ConnectionInfo create(String url, 
ReadOnlyProps props, Properties
 return create(url, conf, props, info);
 }
 
+public static ConnectionInfo create(String url, ReadOnlyProps props,
+Properties info, boolean isServerConnection)
+throws SQLException {
+Configuration conf = 
HBaseFactoryProvider.getConfigurationFactory().getConfiguration();
+ConnectionInfo connInfo = create(url, conf, props, info);
+connInfo.isServerConnection = isServerConnection;

Review Comment:
   Done. 



-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-02-28 Thread via GitHub 


shahrs87 commented on code in PR #1838:
URL: https://github.com/apache/phoenix/pull/1838#discussion_r1506718473


##
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ConnectionInfo.java:
##
@@ -114,6 +115,15 @@ public static ConnectionInfo create(String url, 
ReadOnlyProps props, Properties
 return create(url, conf, props, info);
 }
 
+public static ConnectionInfo create(String url, ReadOnlyProps props,
+Properties info, boolean isServerConnection)
+throws SQLException {
+Configuration conf = 
HBaseFactoryProvider.getConfigurationFactory().getConfiguration();
+ConnectionInfo connInfo = create(url, conf, props, info);
+connInfo.isServerConnection = isServerConnection;

Review Comment:
   All the other variables are being set via Builder. Lets not break that 
pattern for `isServerConnection`



-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-02-28 Thread via GitHub 


shahrs87 commented on code in PR #1838:
URL: https://github.com/apache/phoenix/pull/1838#discussion_r1506717514


##
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ConnectionInfo.java:
##
@@ -114,6 +115,15 @@ public static ConnectionInfo create(String url, 
ReadOnlyProps props, Properties
 return create(url, conf, props, info);
 }
 
+public static ConnectionInfo create(String url, ReadOnlyProps props,
+Properties info, boolean isServerConnection)
+throws SQLException {
+Configuration conf = 
HBaseFactoryProvider.getConfigurationFactory().getConfiguration();
+ConnectionInfo connInfo = create(url, conf, props, info);
+connInfo.isServerConnection = isServerConnection;

Review Comment:
   I think we should initialize `isServerConnection` the same way we are 
initializing principal, keytab, user and haGroup variables.



-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-02-28 Thread via GitHub 


shahrs87 commented on code in PR #1838:
URL: https://github.com/apache/phoenix/pull/1838#discussion_r1506709112


##
phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixTestDriver.java:
##
@@ -92,7 +93,8 @@ public synchronized Connection connect(String url, Properties 
info) throws SQLEx
 public synchronized ConnectionQueryServices 
getConnectionQueryServices(String url, Properties infoIn) throws SQLException {
 checkClosed();
 final Properties info = PropertiesUtil.deepCopy(infoIn);
-ConnectionInfo connInfo = ConnectionInfo.create(url, null, info);
+boolean isServerConnection = 
Boolean.valueOf(info.getProperty(QueryUtil.IS_SERVER_CONNECTION));
+ConnectionInfo connInfo = ConnectionInfo.create(url, null, info, 
isServerConnection);

Review Comment:
   Why do we want to pass as a separate argument for `isServerConnection`?  It 
is present in `info` object anyways.



-- 
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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[PR] PHOENIX-7243 : Add isServerConnection property to ConnectionInfo [phoenix]

2024-02-28 Thread via GitHub 


palashc opened a new pull request, #1838:
URL: https://github.com/apache/phoenix/pull/1838

   (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-unsubscr...@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org