Copilot commented on code in PR #2589:
URL: https://github.com/apache/phoenix/pull/2589#discussion_r3789330901
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java:
##########
@@ -992,31 +995,19 @@ private ClusterRoleRecord
getClusterRoleRecordFromEndpoint() throws SQLException
// Get the CRR via RSEndpoint for cluster 1
ClusterRoleRecord roleRecord =
GetClusterRoleRecordUtil.fetchClusterRoleRecord(info.getUrl1(),
info.getUrl2(), info.getUrl1(), info.getName(), this, pollerInterval,
properties);
- // If we have unknown role for any cluster then try getting CRR from
cluster 2 endpoint and if
- // we get unknown role from there as well then CRR with higher
adminVersion wins.
- if (roleRecord.hasUnknownRole()) {
- ClusterRoleRecord roleRecordFromPR;
- try {
- roleRecordFromPR =
GetClusterRoleRecordUtil.fetchClusterRoleRecord(info.getUrl1(),
- info.getUrl2(), info.getUrl2(), info.getName(), this,
pollerInterval, properties);
- } catch (Exception e) {
- // As we were able to get CRR from cluster 1 but cluster 2 threw
exception then just
- // return
- // CRR from cluster 1 and consume this exception
- LOG.warn("Role Record from cluster {} has Unknown Role but cluster
{} threw exception, "
- + "returning {} as CRR", info.getUrl1(), info.getUrl2(),
roleRecord.toPrettyString());
- return roleRecord;
- }
- if (roleRecordFromPR.hasUnknownRole()) {
- return roleRecord.getVersion() > roleRecordFromPR.getVersion()
- ? roleRecord
- : roleRecordFromPR;
- } else {
- return roleRecordFromPR;
- }
- } else {
+ // Reconcile with cluster 2; if it is unreachable, keep cluster 1's
record.
+ ClusterRoleRecord roleRecordFromPR;
+ try {
+ roleRecordFromPR =
GetClusterRoleRecordUtil.fetchClusterRoleRecord(info.getUrl1(),
+ info.getUrl2(), info.getUrl2(), info.getName(), this,
pollerInterval, properties);
Review Comment:
This second call is not a read-only reconciliation fetch:
`fetchClusterRoleRecord` schedules a poller whenever that endpoint returns a
FAILOVER record with no ACTIVE role (`GetClusterRoleRecordUtil.java:176-186`).
Thus an ACTIVE cluster-1 record plus a stale non-active cluster-2 record now
starts a poller even though reconciliation keeps the ACTIVE record. Besides
extra executor/RPC churn, this expands the existing lock inversion: the poller
calls `refreshClusterRoleRecord(true)` while holding `pollerLock`
(`GetClusterRoleRecordUtil.java:254-261`), while this refresh holds the
HA-group write lock and can block trying to re-enter `schedulePoller`,
producing a deadlock. Fetch both records without scheduling, reconcile them,
and only then schedule a poller if the selected record is actually non-active.
--
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]
