LZD-PratyushBhatt commented on code in PR #3038:
URL: https://github.com/apache/helix/pull/3038#discussion_r2080864925
##########
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:
This should be enough as ContainerManager is the service handling the
deletion.
Regarding the concern, my thoughts are like this:
In the implementation, we're only modifying the behavior of the getElapsed()
method in the ContainerManager, not changing how the ZkServer itself perceives
time. The ZkServer continues to use system time for its operations, session
management, etc.
What we're doing is specifically altering how the ContainerManager
calculates elapsed time for TTL nodes. This is a targeted change that affects
only the container/TTL node cleanup mechanism.
Also, just to mention, this is the same way TTL znodes are cleaned up in
tests in zK repo as well.
##########
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:
Verified this. containerManager.checkContainers() is sync so shouldn't be
an issue.
--
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]
