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