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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
