nzw921rx commented on PR #10808:
URL: https://github.com/apache/seatunnel/pull/10808#issuecomment-4933073234
Thanks for working on this. I re-reviewed the latest head
c9b7f6d8bcf6d7cdde0e4ebdd5c5fcc82a3b605a, with particular attention to
coordinator outages and incomplete membership views.
The overall direction is correct: SeaTunnel control-plane operations should
target a coordinator-capable member rather than blindly following Hazelcast
mastership. However, I found one failover correctness issue in the fallback
behavior.
Blocking issue: an unavailable coordinator is currently represented as a
lite worker
NodeEngineUtil.getActiveMasterAddress() currently falls back to the
Hazelcast master address when no non-lite member is found:
return nodeEngine.getClusterService().getMembers().stream()
.filter(member -> !member.isLiteMember())
.map(Member::getAddress)
.findFirst()
.orElse(hazelcastMasterAddress);
This can occur during a real coordinator outage or membership-transition
window:
all coordinator nodes are temporarily unavailable
-> worker-only lite members remain in the cluster
-> Hazelcast mastership points to a lite worker
-> no non-lite member exists in the current membership view
-> getActiveMasterAddress() returns the lite worker
-> master-bound operations are sent to a node that cannot coordinate them
Control-plane operations cannot succeed until a coordinator returns, but
falling back to the lite worker incorrectly identifies it as the active
coordinator. This may turn a retryable availability gap into an operation-level
failure on the worker.
The same issue exists when getMasterAddress() is non-null but
getMember(masterAddress) temporarily returns null. The current implementation
assumes that unknown member is coordinator-capable:
if (hazelcastMaster == null || !hazelcastMaster.isLiteMember()) {
return hazelcastMasterAddress;
}
During membership convergence, that address may be stale or absent from the
local member view and should not be treated as a confirmed coordinator.
Suggested fix
Return null when no confirmed coordinator-capable member exists:
if (hazelcastMaster == null) {
return null;
}
return nodeEngine.getClusterService().getMembers().stream()
.filter(member -> !member.isLiteMember())
.map(Member::getAddress)
.findFirst()
.orElse(null);
getActiveMasterAddressOrThrow() can then raise the existing
RetryableHazelcastException, allowing callers to retry until a coordinator
becomes available.
Please also add tests covering:
* lite Hazelcast master with no non-lite members;
* non-null master address whose member metadata is unavailable;
* coordinator disappearance followed by a replacement coordinator joining.
Conclusion: can merge after fixes
The centralized coordinator-selection approach is good, but the
no-coordinator fallback should preserve retryable semantics instead of routing
control-plane operations to a lite worker.
--
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]
