GrantPSpencer commented on code in PR #3038:
URL: https://github.com/apache/helix/pull/3038#discussion_r2079091994
##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/RealmAwareZkClientFactoryTestBase.java:
##########
@@ -172,6 +180,12 @@ public void testRealmAwareZkClientCreateWithTTL() {
Assert.assertEquals(DUMMY_RECORD.getSimpleField("Dummy"),
retrievedRecord.getSimpleField("Dummy"));
+ // Check if the TTL znode expires or not.
+ advanceFakeElapsedTime(2000);
+ ContainerManager containerManager =
_zkServerContainerManagerMap.get(_zkServerMap.get(ZkTestBase.ZK_ADDR));
+ containerManager.checkContainers();
+ Assert.assertFalse(_realmAwareZkClient.exists(childPath));
Review Comment:
Generally we should have some verifier for events that are dependent on the
ZK server as there can be flakiness on a single assertion vs assertion retry
with timeout.
These tests are not concerned with performance of ZK server so should allow
ZK server to perform slowly, which may be the case on the github runners
##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/ZkTestBase.java:
##########
@@ -115,6 +134,35 @@ private void setupZooKeepers() {
for (int i = 0; i < _numZk; i++) {
String zkAddress = ZK_PREFIX + (ZK_START_PORT + i);
_zkServerMap.computeIfAbsent(zkAddress, ZkTestBase::startZkServer);
+
_zkServerContainerManagerMap.computeIfAbsent(_zkServerMap.get(zkAddress),
ZkTestBase::createContainerManager);
+ }
+ }
+
+ /**
+ * Creates a ContainerManager with custom elapsed time functionality for a
ZkServer
+ */
+ private static ContainerManager createContainerManager(ZkServer zkServer) {
Review Comment:
is instantiating the container manager itself enough to ensure that the zk
server will clean up expired TTL nodes? I have some concerns with having a fake
timer that we can advance. Diverging the ZkServer's view of time from the
controller's view of time could cause strange behaviors to occur if there is
any logic that is dependent on both.
If clean up will not happen itself, maybe it is best to just expose API to
trigger the cleanup for any nodes that have passed TTL
The tests themselves can handle setting a suitable timeout and then waiting
for that timeout themself
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
