Skip to content

IGNITE-28013 Fix stale leaseholder ID handling in LeaseUpdater and lease batch serialization#7769

Open
Lalant wants to merge 5 commits intoapache:mainfrom
Lalant:IGNITE-28013-lease-must-not-be-prolonged-for-outdated-node
Open

IGNITE-28013 Fix stale leaseholder ID handling in LeaseUpdater and lease batch serialization#7769
Lalant wants to merge 5 commits intoapache:mainfrom
Lalant:IGNITE-28013-lease-must-not-be-prolonged-for-outdated-node

Conversation

@Lalant
Copy link
Contributor

@Lalant Lalant commented Mar 12, 2026

@Lalant Lalant force-pushed the IGNITE-28013-lease-must-not-be-prolonged-for-outdated-node branch from c05b98d to 6941fb7 Compare March 13, 2026 06:53
Comment on lines +122 to +127
for (LogicalNode node : logicalTopologySnap0.nodes()) {
if (node.id().equals(nodeId)) {
return true;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably, this method and nodeByConsistentId should be refactored

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix

Comment on lines +498 to +515
if (hasStaleLeaseholderId) {
LOG.info("Leaseholder has left the logical topology, creating a new lease [groupId={}, lease={}, candidate={}]",
grpId, lease, candidate);

if (candidate == null) {
logGroupWithoutCandidateOnce(grpId, true, stableAssignments, pendingAssignments);
continue;
}

Lease newLease = writeNewLease(grpId, candidate, renewedLeases);

boolean force = Objects.equals(lease.getLeaseholder(), candidate.name());

toBeNegotiated.put(grpId, new LeaseAgreement(newLease, force));

continue;
}

Copy link
Contributor

@denis-chudov denis-chudov Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, all you need is to exclude the outcome when tryToFindCandidateAmongAssignments returns stale candidate, that would be much more simple

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can, but that leaves a known-stale lease published longer than necessary.

Lalant added 3 commits March 16, 2026 16:27
…the issue we change serialization. Moreover, added test to check that non-expired leases are not overriden.

private static boolean holderIdAndProposedCandidateFitIn1ByteForRead(byte protoVer, NodesDictionary dictionary) {
if (protoVer == PROTOCOL_V1) {
return dictionary.nameCount() <= MAX_NODES_FOR_COMPACT_MODE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining that in v1 we assumed that name and node tables have the same size, hence we were only checking for name table size

}

private static byte[] v1BytesWithExactly8NamesAndMoreThan8NodeIdsUsingCompactEncoding() throws IOException {
try (IgniteUnsafeDataOutput out = new IgniteUnsafeDataOutput(256)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain in a comment why here the binary representation is produced 'by hand' (probably because it was not possible to produce it with assertions enabled?)

return out.array();
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test verifying that object serialized with v2 can be deserialized

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants