sk0x50 commented on code in PR #7541:
URL: https://github.com/apache/ignite-3/pull/7541#discussion_r2773485936
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/leases/LeaseTracker.java:
##########
@@ -176,8 +176,8 @@ public Lease getLease(ReplicationGroupId grpId) {
return lease == null ? emptyLease(grpId) : lease;
}
- /** Returns collection of leases, ordered by replication group. */
- public Leases leasesCurrent() {
+ /** Returns collection of latest leases, ordered by replication group.
Shows all latest leases including expired ones. */
+ public Leases leasesLatest() {
Review Comment:
It seems a little weird to me. Maybe it should be renamed to
`latestLeases()` or even `latest()`.
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/metrics/PlacementDriverMetricSource.java:
##########
@@ -90,9 +95,11 @@ public Iterable<Metric> metrics() {
private int numberOfLeases(boolean accepted) {
int count = 0;
+ HybridTimestamp now = clockService.current();
- for (Lease lease :
leaseTracker.leasesCurrent().leaseByGroupId().values()) {
- if (lease != null && accepted == lease.isAccepted()) {
+ for (Lease lease :
leaseTracker.leasesLatest().leaseByGroupId().values()) {
+ // Expired leases can be ignored.
+ if (lease != null && accepted == lease.isAccepted() &&
clockService.before(lease.getExpirationTime(), now)) {
Review Comment:
Is this condition `lease == null` even possible? Once bitten, twice shy :)
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/LeaseUpdater.java:
##########
@@ -500,19 +492,23 @@ private void updateLeaseBatchInternal() {
&& lease.isProlongable()
&& candidate != null &&
candidate.id().equals(lease.getLeaseholderId());
- // The lease is expired or close to this.
+ // The lease is expired or close to this, trying to prolong if
possible or create a new one.
if (lease.getExpirationTime().getPhysical() <
outdatedLeaseThreshold) {
- // If we couldn't find a candidate neither stable nor
pending assignments set, so update stats and skip iteration
+ boolean isLeaseOutdated = isLeaseOutdated(lease);
+
+ // If we couldn't find a candidate neither stable nor
pending assignments set, so skip iteration.
if (candidate == null) {
- leaseWithoutCandidateCount++;
+ logGroupWithoutCandidateOnce(grpId, isLeaseOutdated,
stableAssignments, pendingAssignments);
continue;
}
// We can't prolong the expired lease because we already
have an interval of time when the lease was not active,
// so we must start a negotiation round from the
beginning; the same we do for the groups that don't have
// leaseholders at all.
- if (isLeaseOutdated(lease)) {
+ if (isLeaseOutdated) {
+ LOG.info("Lease is expired, creating a new one
[groupId={}, lease={}, candidate={}]", grpId, lease, candidate);
Review Comment:
Minor. Potentially, we can flood the log file, can we?
--
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]
