sanpwc commented on code in PR #1871:
URL: https://github.com/apache/ignite-3/pull/1871#discussion_r1156119489
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -87,13 +95,22 @@
@ExtendWith(ConfigurationExtension.class)
public class PlacementDriverManagerTest extends IgniteAbstractTest {
public static final int PORT = 1234;
+
+ /** Placement driver message factory. */
+ private static final PlacementDriverMessagesFactory
PLACEMENT_DRIVER_MESSAGES_FACTORY = new PlacementDriverMessagesFactory();
private String nodeName;
+ /** Another node name. The node name is matched to {@code
anotherClusterService}. */
+ private String anotherNodeName;
+
HybridClock clock = new HybridClockImpl();
Review Comment:
It's not related to given proposal, but could you please make clock private
and add an empty line?
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -87,13 +95,22 @@
@ExtendWith(ConfigurationExtension.class)
public class PlacementDriverManagerTest extends IgniteAbstractTest {
public static final int PORT = 1234;
+
+ /** Placement driver message factory. */
+ private static final PlacementDriverMessagesFactory
PLACEMENT_DRIVER_MESSAGES_FACTORY = new PlacementDriverMessagesFactory();
private String nodeName;
Review Comment:
Empty line is missing.
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -111,11 +128,18 @@ public class PlacementDriverManagerTest extends
IgniteAbstractTest {
private TestInfo testInfo;
+ /** This closure handles {@link LeaseGrantedMessage} to check the
placement driver manager behavior. */
+ private BiFunction<LeaseGrantedMessage, String,
LeaseGrantedMessageResponse> leaseGrantHandler;
+
@BeforeEach
public void beforeTest(TestInfo testInfo) throws NodeStoppingException {
this.nodeName = testNodeName(testInfo, PORT);
+ this.anotherNodeName = testNodeName(testInfo, PORT + 1);
this.testInfo = testInfo;
+ assertTrue(nodeName.hashCode() < anotherNodeName.hashCode(),
Review Comment:
Do we have such order guaranties on nodeName.hashCodes?
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -87,13 +95,22 @@
@ExtendWith(ConfigurationExtension.class)
public class PlacementDriverManagerTest extends IgniteAbstractTest {
public static final int PORT = 1234;
+
+ /** Placement driver message factory. */
Review Comment:
Useless javadoc. Constant name is self descriptive here.
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -321,6 +449,45 @@ private void checkLeaseCreated(TablePartitionId grpPartId)
throws InterruptedExc
}, 10_000));
}
+ /**
+ * Checks that the lease for the group is accepted.
+ *
+ * @param grpPartId Replication group id.
+ * @throws InterruptedException If the waiting is interrupted.
+ */
+ private void checkAccepted(TablePartitionId grpPartId) throws
InterruptedException {
+ assertTrue(waitForCondition(() -> {
+ var leaseFut =
metaStorageManager.get(fromString(PLACEMENTDRIVER_PREFIX + grpPartId));
+
+ var leaseEntry = leaseFut.join();
+
+ if (leaseEntry != null && !leaseEntry.empty()) {
+ Lease lease = ByteUtils.fromBytes(leaseEntry.value());
+
+ return lease.isAccepted();
+ }
+
+ return false;
+ }, 10_000));
+ }
+
+ /**
+ * Reads a leas from Meta storage. When you call this method the lease is
expected presenting in Meta storage.
Review Comment:
typos
- leas -> lease
- the lease is expected presenting in Meta storage. -> the lease is expected
to be present in Meta storage.
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -288,11 +355,72 @@ public void testLeaseholderUpdate() throws Exception {
Lease lease = ByteUtils.fromBytes(fut.join().value());
- return lease.getLeaseExpirationTime().compareTo(clock.now()) > 0;
+ return lease.getExpirationTime().compareTo(clock.now()) > 0;
}, 10_000));
}
+ @Test
+ public void testLeaseAccepted() throws Exception {
+ TablePartitionId grpPart0 = createTableAssignment();
+
+ checkLeaseCreated(grpPart0);
+
+ checkAccepted(grpPart0);
Review Comment:
Should we rename it to `checkLeaseAccepted`?
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -288,11 +355,72 @@ public void testLeaseholderUpdate() throws Exception {
Lease lease = ByteUtils.fromBytes(fut.join().value());
- return lease.getLeaseExpirationTime().compareTo(clock.now()) > 0;
+ return lease.getExpirationTime().compareTo(clock.now()) > 0;
}, 10_000));
}
+ @Test
+ public void testLeaseAccepted() throws Exception {
+ TablePartitionId grpPart0 = createTableAssignment();
+
+ checkLeaseCreated(grpPart0);
+
+ checkAccepted(grpPart0);
+ }
+
+ @Test
+ public void testLeaseForceAccepted() throws Exception {
+ leaseGrantHandler = (req, sender) ->
+ PLACEMENT_DRIVER_MESSAGES_FACTORY
+ .leaseGrantedMessageResponse()
+ .accepted(req.force())
+ .build();
+
+ TablePartitionId grpPart0 = createTableAssignment();
+
+ checkLeaseCreated(grpPart0);
+
+ checkAccepted(grpPart0);
+ }
+
+ @Test
+ public void testExceptionOnAcceptance() throws Exception {
+ CountDownLatch latch = new CountDownLatch(1);
+
+ leaseGrantHandler = (req, sender) -> {
+ latch.countDown();
+
+ throw new RuntimeException("test");
Review Comment:
Why we need it?
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -321,6 +449,45 @@ private void checkLeaseCreated(TablePartitionId grpPartId)
throws InterruptedExc
}, 10_000));
}
+ /**
+ * Checks that the lease for the group is accepted.
+ *
+ * @param grpPartId Replication group id.
+ * @throws InterruptedException If the waiting is interrupted.
+ */
+ private void checkAccepted(TablePartitionId grpPartId) throws
InterruptedException {
+ assertTrue(waitForCondition(() -> {
Review Comment:
Hmm, just curious. Is it possible to rewrite it with `assertThat(leaseFut,
willBe());` ?
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -111,11 +128,18 @@ public class PlacementDriverManagerTest extends
IgniteAbstractTest {
private TestInfo testInfo;
+ /** This closure handles {@link LeaseGrantedMessage} to check the
placement driver manager behavior. */
+ private BiFunction<LeaseGrantedMessage, String,
LeaseGrantedMessageResponse> leaseGrantHandler;
Review Comment:
Are we using it withing single thread only?
--
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]
