vldpyatkov commented on code in PR #3401:
URL: https://github.com/apache/ignite-3/pull/3401#discussion_r1524354131
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/negotiation/LeaseNegotiator.java:
##########
@@ -88,28 +91,37 @@ public void negotiate(Lease lease, boolean force) {
if (throwable == null) {
assert msg instanceof LeaseGrantedMessageResponse :
"Message type is unexpected [type="
+ msg.getClass().getSimpleName() + ']';
- } else if (!(unwrapCause(throwable) instanceof
NodeStoppingException)) {
- LOG.warn("Lease was not negotiated due to exception
[lease={}]", throwable, lease);
- }
- LeaseGrantedMessageResponse response =
(LeaseGrantedMessageResponse) msg;
+ LeaseGrantedMessageResponse response =
(LeaseGrantedMessageResponse) msg;
- fut.complete(response);
+ fut.complete(response);
+ } else {
+ if (!(unwrapCause(throwable) instanceof
NodeStoppingException)) {
+ LOG.warn("Lease was not negotiated due to
exception [lease={}]", throwable, lease);
+ }
- triggerToRenewLeases();
+ fut.complete(NOT_ACCEPTED_RESPONSE);
+ }
});
}
/**
- * Gets a lease agreement or {@code null} if the agreement has not formed
yet.
+ * Gets a lease agreement or {@link LeaseAgreement#UNDEFINED_AGREEMENT} if
the process of agreement is not started yet. Removes
+ * the agreement from the map if it is ready.
*
* @param groupId Replication group id.
* @return Lease agreement.
*/
- public LeaseAgreement negotiated(ReplicationGroupId groupId) {
- LeaseAgreement agreement = leaseToNegotiate.getOrDefault(groupId,
UNDEFINED_AGREEMENT);
+ public LeaseAgreement getAndRemoveIfReady(ReplicationGroupId groupId) {
+ LeaseAgreement[] res = new LeaseAgreement[1];
+
+ leaseToNegotiate.compute(groupId, (k, v) -> {
Review Comment:
If a race is excluded here, I prefer to use plain read and conditinal remove.
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/LeaseUpdater.java:
##########
@@ -368,15 +369,14 @@ private void updateLeaseBatchInternal() {
writeNewLease(grpId, lease, candidate, renewedLeases,
toBeNegotiated);
continue;
+ } else {
+ agreement.checkValid(grpId,
topologyTracker.currentTopologySnapshot(), assignments);
Review Comment:
I would put it up just after getting an agreement with the additional
conditions.
It makes logic simpler.
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/TopologyTracker.java:
##########
@@ -142,15 +146,6 @@ public void onUpdate(LogicalTopologySnapshot topologySnap)
{
} while (!topologySnapRef.compareAndSet(logicalTopologySnap0,
topologySnap));
LOG.debug("Logical topology updated for placement driver
[topologySnap={}]", topologySnap);
-
- triggerToRenewLeases();
}
}
-
- /**
- * Triggers to renew leases forcibly. The method wakes up the monitor of
{@link LeaseUpdater}.
- */
- private void triggerToRenewLeases() {
Review Comment:
Why did you remove it?
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/negotiation/LeaseNegotiator.java:
##########
@@ -42,7 +41,11 @@ public class LeaseNegotiator {
private static final PlacementDriverMessagesFactory
PLACEMENT_DRIVER_MESSAGES_FACTORY = new PlacementDriverMessagesFactory();
- /** Leases ready to accept. */
+ static LeaseGrantedMessageResponse NOT_ACCEPTED_RESPONSE =
PLACEMENT_DRIVER_MESSAGES_FACTORY.leaseGrantedMessageResponse()
Review Comment:
Why don't you use {@code null}, like it is in the undefined agreement?
--
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]
