You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "lokiore (via GitHub)" <gi...@apache.org> on 2025/02/10 18:51:35 UTC
[PR] PHOENIX-7495 :- Support for HBase Registry Implementations in Pheonix HA Connections [phoenix]
lokiore opened a new pull request, #2074:
URL: https://github.com/apache/phoenix/pull/2074
(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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7495 :- Support for HBase Registry Implementations in Phoenix HA Connections [phoenix]
Posted by "lokiore (via GitHub)" <gi...@apache.org>.
lokiore merged PR #2074:
URL: https://github.com/apache/phoenix/pull/2074
--
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
Re: [PR] PHOENIX-7495 :- Support for HBase Registry Implementations in Phoenix HA Connections [phoenix]
Posted by "jpisaac (via GitHub)" <gi...@apache.org>.
jpisaac commented on code in PR #2074:
URL: https://github.com/apache/phoenix/pull/2074#discussion_r2015280232
##########
phoenix-core/src/it/java/org/apache/phoenix/jdbc/FailoverPhoenixConnection2IT.java:
##########
@@ -93,19 +96,19 @@ public void setup() throws Exception {
haGroup = getHighAvailibilityGroup(CLUSTERS.getJdbcHAUrl(), clientProperties);
LOG.info("Initialized haGroup {} with URL {}", haGroup, CLUSTERS.getJdbcHAUrl());
- tableName = testName.getMethodName().toUpperCase();
- CLUSTERS.createTableOnClusterPair(tableName);
+ tableName = RandomStringUtils.randomAlphabetic(10).toUpperCase();
Review Comment:
nit : What do you think about combining the two?
TestMethodName + Random? That way it gives more visibility into test and table it is using?
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ClusterRoleRecord.java:
##########
@@ -77,40 +78,99 @@ public static ClusterRole from(byte[] bytes) {
}
}
+ /**
+ * Enum for HBaseRegistryType being used in current clusterRoleRecord, final connection url
+ * are constructed based on RegistryType and urls stored in clusterRoleRecord
+ */
+ public enum RegistryType {
+ ZK, MASTER, RPC
+ }
+
private final String haGroupName;
private final HighAvailabilityPolicy policy;
- private final String zk1;
+ private final RegistryType registryType;
+ private final String url1;
private final ClusterRole role1;
- private final String zk2;
+ private final String url2;
private final ClusterRole role2;
private final long version;
+ /**
+ * To handle backward compatibility with old ClusterRoleRecords which had zk1 and zk2 as keys
+ * for zk urls, This constructor is only being used {@link ClusterRoleRecord#fromJson} when we
+ * deserialize Cluster Role Record read from ZooKeeper ZNode. If CRR is in old format we will
+ * read zk1 and zk2 and url1 and url2 will be null and if it is in new format zk1 and zk2 will
+ * be null in both cases final url is being stored in url1 and url2
+ * @param haGroupName HighAvailability Group name / CRR name
+ * @param policy Policy used by give CRR
+ * @param registryType {@link RegistryType} to be used for given urls
+ * @param url1 ZK/HMaster url based on registry type for first cluster
+ * @param role1 {@link ClusterRole} which describes the current state of first cluster
+ * @param url2 ZK/HMaster url based on registry type for second cluster
+ * @param role2 {@link ClusterRole} which describes the current state of second cluster
+ * @param version version of a given CRR
+ * @param zk1 ZK url of first cluster when CRR is in old format for backward compatibility
+ * @param zk2 ZK url of second cluster when CRR is in old format for backward compatibility
+ */
@JsonCreator
public ClusterRoleRecord(@JsonProperty("haGroupName") String haGroupName,
- @JsonProperty("policy") HighAvailabilityPolicy policy,
- @JsonProperty("zk1") String zk1, @JsonProperty("role1") ClusterRole role1,
- @JsonProperty("zk2") String zk2, @JsonProperty("role2") ClusterRole role2,
- @JsonProperty("version") long version) {
+ @JsonProperty("policy") HighAvailabilityPolicy policy,
+ @JsonProperty("registryType") RegistryType registryType,
+ @JsonProperty("url1") String url1,
+ @JsonProperty("role1") ClusterRole role1,
+ @JsonProperty("url2") String url2,
+ @JsonProperty("role2") ClusterRole role2,
+ @JsonProperty("version") long version,
+ @JsonProperty("zk1") String zk1,
+ @JsonProperty("zk2") String zk2) {
this.haGroupName = haGroupName;
this.policy = policy;
+ this.registryType = registryType != null ? registryType : RegistryType.ZK;
+
+ String resolvedUrl1 = (url1 != null) ? url1 : zk1; //For Backward Compatibility
+ String resolvedUrl2 = (url2 != null) ? url2 : zk2; //For Backward Compatibility
+
//Do we really need to normalize here ?
- zk1 = JDBCUtil.formatZookeeperUrl(zk1);
- zk2 = JDBCUtil.formatZookeeperUrl(zk2);
+ //We are normalizing to have urls in specific formats for each registryType for getting
+ //accurate comparisons. We are passing registryType as these url most probably won't have
+ //protocol in url, and it might be normalized based to wrong registry type, as we normalize
+ //w.r.t {@link ConnectionInfo.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY},
+ //but considering source of truth of registryType is present in CLusterRoleRecord we are
+ //normalizing based on that.
Review Comment:
Will be good to add in the comments here and in class level comments, what does the normalized form looks like.
##########
phoenix-core/src/it/java/org/apache/phoenix/jdbc/ClusterRoleRecordGeneratorToolIT.java:
##########
@@ -104,8 +104,8 @@ public void testListAllRecordsByGenerator1() throws Exception {
assertEquals(1, records.size());
for (ClusterRoleRecord record : records) {
assertEquals(HighAvailabilityPolicy.FAILOVER, record.getPolicy());
- assertEquals(ClusterRole.ACTIVE, record.getRole(CLUSTERS.getUrl1()));
- assertEquals(ClusterRole.STANDBY, record.getRole(CLUSTERS.getUrl2()));
+ assertEquals(ClusterRole.ACTIVE, record.getRole(CLUSTERS.getZkUrl1()));
+ assertEquals(ClusterRole.STANDBY, record.getRole(CLUSTERS.getZkUrl2()));
Review Comment:
Do you want to add some tests around new and old CRR compatibility?
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ClusterRoleRecord.java:
##########
@@ -77,40 +78,99 @@ public static ClusterRole from(byte[] bytes) {
}
}
+ /**
+ * Enum for HBaseRegistryType being used in current clusterRoleRecord, final connection url
+ * are constructed based on RegistryType and urls stored in clusterRoleRecord
+ */
+ public enum RegistryType {
+ ZK, MASTER, RPC
+ }
+
private final String haGroupName;
private final HighAvailabilityPolicy policy;
- private final String zk1;
+ private final RegistryType registryType;
+ private final String url1;
private final ClusterRole role1;
- private final String zk2;
+ private final String url2;
private final ClusterRole role2;
private final long version;
+ /**
+ * To handle backward compatibility with old ClusterRoleRecords which had zk1 and zk2 as keys
+ * for zk urls, This constructor is only being used {@link ClusterRoleRecord#fromJson} when we
+ * deserialize Cluster Role Record read from ZooKeeper ZNode. If CRR is in old format we will
+ * read zk1 and zk2 and url1 and url2 will be null and if it is in new format zk1 and zk2 will
+ * be null in both cases final url is being stored in url1 and url2
+ * @param haGroupName HighAvailability Group name / CRR name
+ * @param policy Policy used by give CRR
+ * @param registryType {@link RegistryType} to be used for given urls
+ * @param url1 ZK/HMaster url based on registry type for first cluster
+ * @param role1 {@link ClusterRole} which describes the current state of first cluster
+ * @param url2 ZK/HMaster url based on registry type for second cluster
+ * @param role2 {@link ClusterRole} which describes the current state of second cluster
+ * @param version version of a given CRR
+ * @param zk1 ZK url of first cluster when CRR is in old format for backward compatibility
+ * @param zk2 ZK url of second cluster when CRR is in old format for backward compatibility
+ */
@JsonCreator
public ClusterRoleRecord(@JsonProperty("haGroupName") String haGroupName,
- @JsonProperty("policy") HighAvailabilityPolicy policy,
- @JsonProperty("zk1") String zk1, @JsonProperty("role1") ClusterRole role1,
- @JsonProperty("zk2") String zk2, @JsonProperty("role2") ClusterRole role2,
- @JsonProperty("version") long version) {
+ @JsonProperty("policy") HighAvailabilityPolicy policy,
+ @JsonProperty("registryType") RegistryType registryType,
+ @JsonProperty("url1") String url1,
+ @JsonProperty("role1") ClusterRole role1,
+ @JsonProperty("url2") String url2,
+ @JsonProperty("role2") ClusterRole role2,
+ @JsonProperty("version") long version,
+ @JsonProperty("zk1") String zk1,
+ @JsonProperty("zk2") String zk2) {
this.haGroupName = haGroupName;
this.policy = policy;
+ this.registryType = registryType != null ? registryType : RegistryType.ZK;
+
+ String resolvedUrl1 = (url1 != null) ? url1 : zk1; //For Backward Compatibility
+ String resolvedUrl2 = (url2 != null) ? url2 : zk2; //For Backward Compatibility
Review Comment:
Can resolvedUrls be nulls under any case? When starting fresh or misconfigured?
Need to ensure that resolved urls are never null.
Maybe good to add tests to that effect.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityPolicy.java:
##########
@@ -123,15 +158,88 @@ public Connection provide(HighAvailabilityGroup haGroup, Properties info,
return haGroup.connectActive(info, haURLInfo);
}
}
+
@Override
void transitClusterRole(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
ClusterRoleRecord newRecord) {
- LOG.info("Cluster role changed for parallel HA policy.");
+ //No Action for Parallel Policy
+ }
+
+
+ @Override
+ void transitRoleRecordRegistry(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
+ ClusterRoleRecord newRecord) throws SQLException {
+ //Close connections of both clusters as there is a change in registryType
+ LOG.info("Cluster {} and {} has a change in registryType in HA group {}, now closing all its connections",
+ oldRecord.getUrl1(), oldRecord.getUrl2() , oldRecord.getRegistryType());
+ //close connections for cluster 1
+ closeConnections(haGroup, oldRecord.getUrl1(), oldRecord.getRegistryType());
+ //close connections for cluster 2
+ closeConnections(haGroup, oldRecord.getUrl2(), oldRecord.getRegistryType());
+ }
+
+ /**
+ * For PARALLEL policy if there is a change in any of the url then ParallelPhoenixConnection
+ * objects are invalid
+ * @param haGroup The high availability (HA) group
+ * @param oldRecord The older cluster role record cached in this client for the given HA group
+ * @param newRecord New cluster role record read from one ZooKeeper cluster znode
+ * @throws SQLException when not able to close connections
+ */
+ @Override
+ void transitClusterUrl(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
+ ClusterRoleRecord newRecord) throws SQLException {
+ if (!Objects.equals(oldRecord.getUrl1(), newRecord.getUrl1()) &&
+ !Objects.equals(oldRecord.getUrl1(), newRecord.getUrl2())) {
+ LOG.info("Cluster {} is changed to {} in HA group {}, now closing all its connections",
+ oldRecord.getUrl1(), newRecord.getUrl1(), haGroup);
+ closeConnections(haGroup, oldRecord.getUrl1(), oldRecord.getRegistryType());
+ }
+ if (!Objects.equals(oldRecord.getUrl2(), newRecord.getUrl2()) &&
+ !Objects.equals(oldRecord.getUrl2(), newRecord.getUrl1())) {
+ LOG.info("Cluster {} is changed to {} in HA group {}, now closing all its connections",
+ oldRecord.getUrl2(), newRecord.getUrl2(), haGroup);
+ closeConnections(haGroup, oldRecord.getUrl2(), oldRecord.getRegistryType());
+ }
+
}
};
private static final Logger LOG = LoggerFactory.getLogger(HighAvailabilityGroup.class);
+ /**
+ * Utility to close cqs and all it's connection for specific url of a HAGroup
+ * @param haGroup The High Availability (HA) Group
+ * @param url The url on which cqs and connections are present
+ * @param registryType The registry Type of connections
+ * @throws SQLException if fails to close the connections
+ */
+ private static void closeConnections(HighAvailabilityGroup haGroup, String url,
+ ClusterRoleRecord.RegistryType registryType) throws SQLException {
+ ConnectionQueryServices cqs = null;
+ //Close connections for every HAURLInfo's (different principal) conn for a give HAGroup
+ for (HAURLInfo haurlInfo : HighAvailabilityGroup.URLS.get(haGroup.getGroupInfo())) {
+ try {
+ cqs = PhoenixDriver.INSTANCE.getConnectionQueryServices(
+ HighAvailabilityGroup.getJDBCUrl(url, haurlInfo, registryType), haGroup.getProperties());
+ cqs.closeAllConnections(new SQLExceptionInfo
+ .Builder(SQLExceptionCode.HA_CLOSED_AFTER_FAILOVER)
+ .setMessage("Phoenix connection got closed due to failover")
+ .setHaGroupInfo(haGroup.getGroupInfo().toString()));
+ LOG.info("Closed all connections to cluster {} for HA group {}", url,
+ haGroup.getGroupInfo());
+ } finally {
+ if (cqs != null) {
+ // CQS is closed, but it is not invalidated from global cache in PhoenixDriver
+ // so that any new connection will get error instead of creating a new CQS
+ LOG.info("Closing CQS after clusterRoleRecord change for '{}'", url);
+ cqs.close();
Review Comment:
Since we are leaving it in the map until the cache is repopulated. Good to add as a comment.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ClusterRoleRecord.java:
##########
@@ -77,40 +78,99 @@ public static ClusterRole from(byte[] bytes) {
}
}
+ /**
+ * Enum for HBaseRegistryType being used in current clusterRoleRecord, final connection url
+ * are constructed based on RegistryType and urls stored in clusterRoleRecord
+ */
+ public enum RegistryType {
+ ZK, MASTER, RPC
+ }
+
private final String haGroupName;
private final HighAvailabilityPolicy policy;
- private final String zk1;
+ private final RegistryType registryType;
+ private final String url1;
private final ClusterRole role1;
- private final String zk2;
+ private final String url2;
private final ClusterRole role2;
private final long version;
+ /**
+ * To handle backward compatibility with old ClusterRoleRecords which had zk1 and zk2 as keys
+ * for zk urls, This constructor is only being used {@link ClusterRoleRecord#fromJson} when we
+ * deserialize Cluster Role Record read from ZooKeeper ZNode. If CRR is in old format we will
+ * read zk1 and zk2 and url1 and url2 will be null and if it is in new format zk1 and zk2 will
+ * be null in both cases final url is being stored in url1 and url2
+ * @param haGroupName HighAvailability Group name / CRR name
+ * @param policy Policy used by give CRR
+ * @param registryType {@link RegistryType} to be used for given urls
+ * @param url1 ZK/HMaster url based on registry type for first cluster
+ * @param role1 {@link ClusterRole} which describes the current state of first cluster
+ * @param url2 ZK/HMaster url based on registry type for second cluster
+ * @param role2 {@link ClusterRole} which describes the current state of second cluster
+ * @param version version of a given CRR
+ * @param zk1 ZK url of first cluster when CRR is in old format for backward compatibility
+ * @param zk2 ZK url of second cluster when CRR is in old format for backward compatibility
+ */
@JsonCreator
public ClusterRoleRecord(@JsonProperty("haGroupName") String haGroupName,
- @JsonProperty("policy") HighAvailabilityPolicy policy,
- @JsonProperty("zk1") String zk1, @JsonProperty("role1") ClusterRole role1,
- @JsonProperty("zk2") String zk2, @JsonProperty("role2") ClusterRole role2,
- @JsonProperty("version") long version) {
+ @JsonProperty("policy") HighAvailabilityPolicy policy,
+ @JsonProperty("registryType") RegistryType registryType,
+ @JsonProperty("url1") String url1,
+ @JsonProperty("role1") ClusterRole role1,
+ @JsonProperty("url2") String url2,
+ @JsonProperty("role2") ClusterRole role2,
+ @JsonProperty("version") long version,
+ @JsonProperty("zk1") String zk1,
+ @JsonProperty("zk2") String zk2) {
this.haGroupName = haGroupName;
this.policy = policy;
+ this.registryType = registryType != null ? registryType : RegistryType.ZK;
+
+ String resolvedUrl1 = (url1 != null) ? url1 : zk1; //For Backward Compatibility
+ String resolvedUrl2 = (url2 != null) ? url2 : zk2; //For Backward Compatibility
+
//Do we really need to normalize here ?
- zk1 = JDBCUtil.formatZookeeperUrl(zk1);
- zk2 = JDBCUtil.formatZookeeperUrl(zk2);
+ //We are normalizing to have urls in specific formats for each registryType for getting
+ //accurate comparisons. We are passing registryType as these url most probably won't have
+ //protocol in url, and it might be normalized based to wrong registry type, as we normalize
+ //w.r.t {@link ConnectionInfo.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY},
+ //but considering source of truth of registryType is present in CLusterRoleRecord we are
+ //normalizing based on that.
+ url1 = JDBCUtil.formatUrl(resolvedUrl1, this.registryType);
+ url2 = JDBCUtil.formatUrl(resolvedUrl2, this.registryType);
+
+ Preconditions.checkArgument(!url1.equals(url2), "Two clusters have the same URLS!");
// Ignore the given order of url1 and url2
- if (zk1.compareTo(zk2) < 0) {
- this.zk1 = zk1;
+ if (url1.compareTo(url2) < 0) {
+ this.url1 = url1;
this.role1 = role1;
- this.zk2 = zk2;
+ this.url2 = url2;
this.role2 = role2;
} else {
- this.zk1 = zk2;
+ this.url1 = url2;
this.role1 = role2;
- this.zk2 = zk1;
+ this.url2 = url1;
this.role2 = role1;
}
this.version = version;
}
+ public ClusterRoleRecord(String haGroupName, HighAvailabilityPolicy policy,
+ String url1, ClusterRole role1,
+ String url2, ClusterRole role2,
+ long version) {
+ this(haGroupName, policy, RegistryType.ZK, url1, role1, url2, role2, version, null, null);
+ }
+
+ public ClusterRoleRecord(String haGroupName, HighAvailabilityPolicy policy,
+ RegistryType registryType,
+ String url1, ClusterRole role1,
+ String url2, ClusterRole role2,
+ long version) {
+ this(haGroupName, policy, registryType, url1, role1, url2, role2, version, null, null);
+ }
+
public static Optional<ClusterRoleRecord> fromJson(byte[] bytes) {
if (bytes == null) {
return Optional.empty();
Review Comment:
It might be prudent to return Optional if the CRR does not confirm to all the validations, like urls, policy and roles are not set. Right?
--
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
Re: [PR] PHOENIX-7495 :- Support for HBase Registry Implementations in Pheonix HA Connections [phoenix]
Posted by "jpisaac (via GitHub)" <gi...@apache.org>.
jpisaac commented on code in PR #2074:
URL: https://github.com/apache/phoenix/pull/2074#discussion_r1986141892
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java:
##########
@@ -399,7 +401,9 @@ static Optional<String> getFallbackCluster(String url, Properties properties) th
}
String fallbackCluster = properties.getProperty(PHOENIX_HA_FALLBACK_CLUSTER_KEY);
if (StringUtils.isEmpty(fallbackCluster)) {
- fallbackCluster = haGroupInfo.getUrl1();
+ LOG.error("Fallback to single cluster is enabled for the HA group {} but cluster key is"
+ + " empty per configuration. HA url: '{}'.", haGroupInfo.getName(), url);
Review Comment:
You may want to add more info in the error log that one cannot use HAGroupInfo url, since they are bootstrap url and can be different from the ZK node urls (source of truth)
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java:
##########
@@ -1030,5 +1050,15 @@ private void nodeChanged() {
roleManagerLatch.countDown();
}
}
+
+ }
+
+ private static boolean anyNotNull(Object... params) {
Review Comment:
Use ObjectUtils.anyNotNull?
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java:
##########
@@ -271,6 +268,11 @@ public static HAURLInfo getUrlInfo(String url, Properties properties) throws SQL
}
}
+ //If additional parameter is only ; then making it null.
+ additionalJDBCParams = additionalJDBCParams != null
+ ? (additionalJDBCParams.equals(PhoenixRuntime.JDBC_PROTOCOL_TERMINATOR)
Review Comment:
Also how does setting additionalJDBCParams help?
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java:
##########
@@ -962,6 +933,55 @@ public int hashCode() {
}
}
+ /**
+ * Helper method to construct the jdbc url back from the given information
+ * @param url contains host and port part of jdbc url, to get ha url in jdbc format
+ * this function can be used, but needs url in ha format already i.e. [url1|url2]
+ * for MASTER and RPC registry
+ * @param haURLInfo contains principal and additional information
+ * @param type Registry Type for which url has to be constructed
+ * @return jdbc url in proper format
+ */
+ public static String getJDBCUrl(String url, HAURLInfo haURLInfo,
Review Comment:
Maybe good idea to comment on the expected incoming and returned format?
##########
phoenix-core-client/src/main/java/org/apache/phoenix/util/JDBCUtil.java:
##########
@@ -208,37 +210,63 @@ public static String getSchema(String url, Properties info, String defaultValue)
}
/**
- * Get the ZK quorom and root and node part of the URL, which is used by the HA code internally
- * to identify the clusters.
+ * Get the ZK quorum and root and node part of the URL in case of ZKRegistry and bootstrap nodes for other registry
+ * which is used by the HA code internally to identify the clusters.
* As we interpret a missing protocol as ZK, this is mostly idempotent for zk quorum strings.
*
- * @param jdbcUrl JDBC URL
- * @return part of the URL determining the ZK quorum and node
+ * @param jdbcUrl JDBC URL with proper protocol present in string
+ * @return part of the URL determining the ZK quorum and node or bootstrapServers
* @throws RuntimeException if the URL is invalid, or does not resolve to a ZK Registry
* connection
*/
- public static String formatZookeeperUrl(String jdbcUrl) {
+ public static String formatUrl(String jdbcUrl) {
ConnectionInfo connInfo;
try {
- Properties info = new Properties();
- // Make sure we use ZK on HBase 3.x
- info.put(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
- ZKConnectionInfo.ZK_REGISTRY_NAME);
- connInfo = ConnectionInfo.create(jdbcUrl, null, info);
- // TODO in theory we could support non-ZK registries for HA.
- // However, as HA already relies on ZK, this wouldn't be particularly useful,
- // and would require significant changes.
- if (!(connInfo instanceof ZKConnectionInfo)) {
- throw new SQLException("HA connections must use ZooKeeper registry. " + jdbcUrl
- + " is not a Zookeeper HBase connection");
- }
- ZKConnectionInfo zkInfo = (ZKConnectionInfo) connInfo;
+ connInfo = ConnectionInfo.create(jdbcUrl, null, null);
StringBuilder sb = new StringBuilder();
- sb.append(zkInfo.getZkHosts().replaceAll(":", "\\\\:")).append("::")
- .append(zkInfo.getZkRootNode());
+ if (connInfo instanceof AbstractRPCConnectionInfo) {
+ //TODO: check if anything else is needed for RPCRegistry connections and do we need to store them in CRR
+ AbstractRPCConnectionInfo rpcInfo = (AbstractRPCConnectionInfo) connInfo;
+ sb.append(rpcInfo.getBoostrapServers().replaceAll(":", "\\\\:"));
+ } else {
+ ZKConnectionInfo zkInfo = (ZKConnectionInfo) connInfo;
+ sb.append(zkInfo.getZkHosts().replaceAll(":", "\\\\:")).append("::")
Review Comment:
Why the "::" append? May be add a comment why it is needed?
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java:
##########
@@ -962,6 +933,55 @@ public int hashCode() {
}
}
+ /**
+ * Helper method to construct the jdbc url back from the given information
+ * @param url contains host and port part of jdbc url, to get ha url in jdbc format
+ * this function can be used, but needs url in ha format already i.e. [url1|url2]
+ * for MASTER and RPC registry
+ * @param haURLInfo contains principal and additional information
+ * @param type Registry Type for which url has to be constructed
+ * @return jdbc url in proper format
+ */
+ public static String getJDBCUrl(String url, HAURLInfo haURLInfo,
Review Comment:
Do we need to validate the incoming url? Or is there any validation upstream?
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java:
##########
@@ -271,6 +268,11 @@ public static HAURLInfo getUrlInfo(String url, Properties properties) throws SQL
}
}
+ //If additional parameter is only ; then making it null.
+ additionalJDBCParams = additionalJDBCParams != null
+ ? (additionalJDBCParams.equals(PhoenixRuntime.JDBC_PROTOCOL_TERMINATOR)
Review Comment:
Need proper equals checking or validate this equals works. JDBC_PROTOCOL_TERMINATOR is char type.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ClusterRoleRecord.java:
##########
@@ -77,40 +78,81 @@ public static ClusterRole from(byte[] bytes) {
}
}
+ /**
+ * Enum for HBaseRegistryType being used in current clusterRoleRecord, final connection url
+ * are constructed based on RegistryType and urls stored in clusterRoleRecord
+ */
+ public enum RegistryType {
+ ZK, MASTER, RPC
+ }
+
private final String haGroupName;
private final HighAvailabilityPolicy policy;
- private final String zk1;
+ private final RegistryType registryType;
+ private final String url1;
private final ClusterRole role1;
- private final String zk2;
+ private final String url2;
private final ClusterRole role2;
private final long version;
@JsonCreator
public ClusterRoleRecord(@JsonProperty("haGroupName") String haGroupName,
- @JsonProperty("policy") HighAvailabilityPolicy policy,
- @JsonProperty("zk1") String zk1, @JsonProperty("role1") ClusterRole role1,
- @JsonProperty("zk2") String zk2, @JsonProperty("role2") ClusterRole role2,
- @JsonProperty("version") long version) {
+ @JsonProperty("policy") HighAvailabilityPolicy policy,
+ @JsonProperty("registryType") RegistryType registryType,
+ @JsonProperty("url1") String url1,
+ @JsonProperty("role1") ClusterRole role1,
+ @JsonProperty("url2") String url2,
+ @JsonProperty("role2") ClusterRole role2,
+ @JsonProperty("version") long version,
+ @JsonProperty("zk1") String zk1, //Deprecated
+ @JsonProperty("zk2") String zk2) {
this.haGroupName = haGroupName;
this.policy = policy;
+ this.registryType = registryType != null ? registryType : RegistryType.ZK;
+
+ String resolvedUrl1 = (url1 != null) ? url1 : zk1; //For Backward Compatibility
+ String resolvedUrl2 = (url2 != null) ? url2 : zk2; //For Backward Compatibility
Review Comment:
Can you add some comment to backward compatibility issue we are handling? Seems like zk1 and zk2 are always null.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ClusterRoleRecord.java:
##########
@@ -77,40 +78,81 @@ public static ClusterRole from(byte[] bytes) {
}
}
+ /**
+ * Enum for HBaseRegistryType being used in current clusterRoleRecord, final connection url
+ * are constructed based on RegistryType and urls stored in clusterRoleRecord
+ */
+ public enum RegistryType {
+ ZK, MASTER, RPC
+ }
+
private final String haGroupName;
private final HighAvailabilityPolicy policy;
- private final String zk1;
+ private final RegistryType registryType;
+ private final String url1;
private final ClusterRole role1;
- private final String zk2;
+ private final String url2;
private final ClusterRole role2;
private final long version;
@JsonCreator
public ClusterRoleRecord(@JsonProperty("haGroupName") String haGroupName,
- @JsonProperty("policy") HighAvailabilityPolicy policy,
- @JsonProperty("zk1") String zk1, @JsonProperty("role1") ClusterRole role1,
- @JsonProperty("zk2") String zk2, @JsonProperty("role2") ClusterRole role2,
- @JsonProperty("version") long version) {
+ @JsonProperty("policy") HighAvailabilityPolicy policy,
+ @JsonProperty("registryType") RegistryType registryType,
+ @JsonProperty("url1") String url1,
+ @JsonProperty("role1") ClusterRole role1,
+ @JsonProperty("url2") String url2,
+ @JsonProperty("role2") ClusterRole role2,
+ @JsonProperty("version") long version,
+ @JsonProperty("zk1") String zk1, //Deprecated
Review Comment:
Not sure I understand, what is deprecated?
--
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
Re: [PR] PHOENIX-7495 :- Support for HBase Registry Implementations in Pheonix HA Connections [phoenix]
Posted by "lokiore (via GitHub)" <gi...@apache.org>.
lokiore commented on PR #2074:
URL: https://github.com/apache/phoenix/pull/2074#issuecomment-2651909513
> General comment, these HA IT tests are extremely heavy... I'd recommend we don't parameterize all these tests.
@dbwong yes I was checking how much time it will increase, which seems to be a lot. Will add couple of separate classes with cherry picked tests ✌️
--
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
Re: [PR] PHOENIX-7495 :- Support for HBase Registry Implementations in Pheonix HA Connections [phoenix]
Posted by "jpisaac (via GitHub)" <gi...@apache.org>.
jpisaac commented on code in PR #2074:
URL: https://github.com/apache/phoenix/pull/2074#discussion_r1994114851
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityPolicy.java:
##########
@@ -44,63 +46,96 @@ public Connection provide(HighAvailabilityGroup haGroup, Properties info,
FailoverPhoenixContext context = new FailoverPhoenixContext(info, haGroup, haURLInfo);
return new FailoverPhoenixConnection(context);
}
+
@Override
void transitClusterRole(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
ClusterRoleRecord newRecord) throws SQLException {
if (oldRecord.getRole1() == ACTIVE && newRecord.getRole1() == STANDBY) {
- transitStandby(haGroup, oldRecord.getZk1());
+ transitStandby(haGroup, oldRecord.getUrl1(), oldRecord.getRegistryType());
}
if (oldRecord.getRole2() == ACTIVE && newRecord.getRole2() == STANDBY) {
- transitStandby(haGroup, oldRecord.getZk2());
+ transitStandby(haGroup, oldRecord.getUrl2(), oldRecord.getRegistryType());
}
if (oldRecord.getRole1() != ACTIVE && newRecord.getRole1() == ACTIVE) {
- transitActive(haGroup, oldRecord.getZk1());
+ transitActive(haGroup, oldRecord.getUrl1(), oldRecord.getRegistryType());
}
if (oldRecord.getRole2() != ACTIVE && newRecord.getRole2() == ACTIVE) {
- transitActive(haGroup, oldRecord.getZk2());
+ transitActive(haGroup, oldRecord.getUrl2(), oldRecord.getRegistryType());
}
}
- private void transitStandby(HighAvailabilityGroup haGroup, String zkUrl)
- throws SQLException {
- // Close connections when a previously ACTIVE HBase cluster becomes STANDBY.
- LOG.info("Cluster {} becomes STANDBY in HA group {}, now close all its connections",
- zkUrl, haGroup.getGroupInfo());
- ConnectionQueryServices cqs = null;
-
- //Close connections for every HAURLInfo's (different principal) conn for a give HAGroup
- for (HAURLInfo haurlInfo : HighAvailabilityGroup.URLS.get(haGroup.getGroupInfo())) {
- try {
- String jdbcZKUrl = haGroup.getGroupInfo().getJDBCUrl(zkUrl, haurlInfo);
- cqs = PhoenixDriver.INSTANCE.getConnectionQueryServices(
- jdbcZKUrl, haGroup.getProperties());
- cqs.closeAllConnections(new SQLExceptionInfo
- .Builder(SQLExceptionCode.HA_CLOSED_AFTER_FAILOVER)
- .setMessage("Phoenix connection got closed due to failover")
- .setHaGroupInfo(haGroup.getGroupInfo().toString()));
- LOG.info("Closed all connections to cluster {} for HA group {}",
- jdbcZKUrl, haGroup.getGroupInfo());
- } finally {
- if (cqs != null) {
- //CQS is closed but it is not invalidated from global cache in PhoenixDriver
- //so that any new connection will get error instead of creating a new CQS
- LOG.info("Closing CQS after cluster '{}' becomes STANDBY", zkUrl);
- cqs.close();
- LOG.info("Successfully closed CQS after cluster '{}' becomes STANDBY",
- zkUrl);
+
+ @Override
+ void transitRoleRecordRegistry(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
+ ClusterRoleRecord newRecord) throws SQLException {
+ Optional<String> activeUrl = oldRecord.getActiveUrl();
+
+ //Close connections for Active HBase cluster as there is a change in Registry Type
+ if (activeUrl.isPresent()) {
+ LOG.info("Cluster {} has a change in registryType in HA group {}, now closing "
+ + "all its connections", activeUrl.get(), oldRecord.getRegistryType());
+ closeConnections(haGroup, activeUrl.get(), oldRecord.getRegistryType());
+ } else {
+ LOG.info("None of the cluster in HA Group {} is active", haGroup);
+ }
+ }
+
+ /**
+ * For FAILOVER Policy if there is a change in active url or no new active url then close all connections.
+ * (url1, ACTIVE, url2, STANDBY) --> (url1, ACTIVE, url3, STANDBY) //Nothing is needed
+ * (url1, ACTIVE, url2, STANDBY) --> (url3, ACTIVE, url2, STANDBY) //Active url change close connections
+ * (url1, ACTIVE, url2, STANDBY) --> (url3, ACTIVE, url4, STANDBY) //Active url change close connections
+ * (url1, ACTIVE, url2, STANDBY) --> (url3, ACTIVE, url1, STANDBY)
+ * //Here active became standby but other url changed close connections
+ * (url1, OFFLINE, url2, STANDBY) --> (url3, ACTIVE, url2, STANDBY) //Nothing to do as there were no connections
+ * (url1, ACTIVE, url2, STANDBY) --> (url3, OFFLINE, url2, STANDBY) //Closing old connections as no new active url
+ * @param haGroup The high availability (HA) group
+ * @param oldRecord The older cluster role record cached in this client for the given HA group
+ * @param newRecord New cluster role record read from one ZooKeeper cluster znode
+ * @throws SQLException when not able to close connections
Review Comment:
Need to explain what url1 ... url4 stand for. Not sure what the transitions standfor?
--
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
Re: [PR] PHOENIX-7495 :- Support for HBase Registry Implementations in Phoenix HA Connections [phoenix]
Posted by "jpisaac (via GitHub)" <gi...@apache.org>.
jpisaac commented on code in PR #2074:
URL: https://github.com/apache/phoenix/pull/2074#discussion_r2017732658
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityPolicy.java:
##########
@@ -44,63 +46,98 @@ public Connection provide(HighAvailabilityGroup haGroup, Properties info,
FailoverPhoenixContext context = new FailoverPhoenixContext(info, haGroup, haURLInfo);
return new FailoverPhoenixConnection(context);
}
+
@Override
void transitClusterRole(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
Review Comment:
As discussed, maybe good to add the check for both roles being ACTIVE, due to misconfiguration and log 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@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7495 :- Support for HBase Registry Implementations in Pheonix HA Connections [phoenix]
Posted by "jpisaac (via GitHub)" <gi...@apache.org>.
jpisaac commented on code in PR #2074:
URL: https://github.com/apache/phoenix/pull/2074#discussion_r1994126520
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityPolicy.java:
##########
@@ -123,15 +158,88 @@ public Connection provide(HighAvailabilityGroup haGroup, Properties info,
return haGroup.connectActive(info, haURLInfo);
}
}
+
@Override
void transitClusterRole(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
ClusterRoleRecord newRecord) {
- LOG.info("Cluster role changed for parallel HA policy.");
+ //No Action for Parallel Policy
+ }
+
+
+ @Override
+ void transitRoleRecordRegistry(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
+ ClusterRoleRecord newRecord) throws SQLException {
+ //Close connections of both clusters as there is a change in registryType
+ LOG.info("Cluster {} and {} has a change in registryType in HA group {}, now closing all its connections",
+ oldRecord.getUrl1(), oldRecord.getUrl2() , oldRecord.getRegistryType());
+ //close connections for cluster 1
+ closeConnections(haGroup, oldRecord.getUrl1(), oldRecord.getRegistryType());
+ //close connections for cluster 2
+ closeConnections(haGroup, oldRecord.getUrl2(), oldRecord.getRegistryType());
+ }
+
+ /**
+ * For PARALLEL policy if there is a change in any of the url then ParallelPhoenixConnection
+ * objects are invalid
+ * @param haGroup The high availability (HA) group
+ * @param oldRecord The older cluster role record cached in this client for the given HA group
+ * @param newRecord New cluster role record read from one ZooKeeper cluster znode
+ * @throws SQLException when not able to close connections
+ */
+ @Override
+ void transitClusterUrl(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
+ ClusterRoleRecord newRecord) throws SQLException {
+ if (!Objects.equals(oldRecord.getUrl1(), newRecord.getUrl1()) &&
+ !Objects.equals(oldRecord.getUrl1(), newRecord.getUrl2())) {
+ LOG.info("Cluster {} is changed to {} in HA group {}, now closing all its connections",
+ oldRecord.getUrl1(), newRecord.getUrl1(), haGroup);
+ closeConnections(haGroup, oldRecord.getUrl1(), oldRecord.getRegistryType());
+ }
+ if (!Objects.equals(oldRecord.getUrl2(), newRecord.getUrl2()) &&
+ !Objects.equals(oldRecord.getUrl2(), newRecord.getUrl1())) {
+ LOG.info("Cluster {} is changed to {} in HA group {}, now closing all its connections",
+ oldRecord.getUrl2(), newRecord.getUrl2(), haGroup);
+ closeConnections(haGroup, oldRecord.getUrl2(), oldRecord.getRegistryType());
+ }
+
}
};
private static final Logger LOG = LoggerFactory.getLogger(HighAvailabilityGroup.class);
+ /**
+ * Utility to close cqs and all it's connection for specific url of a HAGroup
+ * @param haGroup The High Availability (HA) Group
+ * @param url The url on which cqs and connections are present
+ * @param registryType The registry Type of connections
+ * @throws SQLException if fails to close the connections
+ */
+ private static void closeConnections(HighAvailabilityGroup haGroup, String url,
+ ClusterRoleRecord.RegistryType registryType) throws SQLException {
+ ConnectionQueryServices cqs = null;
+ //Close connections for every HAURLInfo's (different principal) conn for a give HAGroup
+ for (HAURLInfo haurlInfo : HighAvailabilityGroup.URLS.get(haGroup.getGroupInfo())) {
+ try {
+ cqs = PhoenixDriver.INSTANCE.getConnectionQueryServices(
+ HighAvailabilityGroup.getJDBCUrl(url, haurlInfo, registryType), haGroup.getProperties());
+ cqs.closeAllConnections(new SQLExceptionInfo
+ .Builder(SQLExceptionCode.HA_CLOSED_AFTER_FAILOVER)
+ .setMessage("Phoenix connection got closed due to failover")
+ .setHaGroupInfo(haGroup.getGroupInfo().toString()));
+ LOG.info("Closed all connections to cluster {} for HA group {}", url,
+ haGroup.getGroupInfo());
+ } finally {
+ if (cqs != null) {
+ // CQS is closed, but it is not invalidated from global cache in PhoenixDriver
+ // so that any new connection will get error instead of creating a new CQS
+ LOG.info("Closing CQS after clusterRoleRecord change for '{}'", url);
+ cqs.close();
Review Comment:
Will also need to remove it from the CQSI map, otherwise the map will contain a closed CQSI and will never initialize again.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityPolicy.java:
##########
@@ -145,7 +253,30 @@ abstract Connection provide(HighAvailabilityGroup haGroup, Properties info, HAUR
throws SQLException;
/**
- * Call-back function when a cluster role transition is detected in the high availability group.
+ * Call-back function when a cluster role record transition is detected in the high availability group.
+ *
+ * @param haGroup The high availability (HA) group
+ * @param oldRecord The older cluster role record cached in this client for the given HA group
+ * @param newRecord New cluster role record read from one ZooKeeper cluster znode
+ * @throws SQLException if fails to handle the cluster role record transition
+ */
+ public void transitClusterRoleRecord(HighAvailabilityGroup haGroup, ClusterRoleRecord oldRecord,
Review Comment:
What about cases when there are multiple changes? Registry types, urls, roles?
--
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