Add unit tests to estore #1

sggeorgiev · 2025-06-20T14:32:39Z

This PR adds unit test for estore functionality.

moticless · 2025-06-23T08:42:25Z

src/expire.c

+
+    for(int i = 0; i < es->num_buckets && maxCascade > 0; i++) 
+        maxCascade -= ebCascade(es->buckets + i, es->bucket_type, now, maxCascade);
+}


We will need some "static" context to memorize across calls , at which bucket we stopped as well and not start from the begining (Later on, we might want increase aggressivness if too many times stopped due to maxCascade limit.)

Need some way to iterate over the non empty buckets, very similar to the way kvstore does. See function kvstoreGetNextNonEmptyDictIndex()

Static context is added.

moticless · 2025-06-23T08:46:33Z

src/expire.c

+    TEST("Create estore with pre-allocated buckets") {
+        int num_bits = 3; // 2^3 = 8 buckets
+        int num_buckets = 1 << num_bits;
+
+        estore *es = estoreCreate(&testEbType, num_bits);
+
+        for (int i = 0; i < num_buckets; i++) {
+            ebuckets b = es->buckets[i];
+            assert(b == NULL); // ebCreate returns NULL
+        }
+
+        assert(es->num_buckets == num_buckets);
+        assert(es->num_buckets_bits == num_bits);
+        assert(es->bucket_type == &testEbType);
+        assert(es->count == 0);
+
+        estoreRelease(es); // Clean up
+    }


This test covers overly trivial logic and seems redundant.
Please try to use c-style comments.

Covering even the trivial test cases helps improve code coverage. Good coverage shows that the code works as expected and is designed properly. It also gives us confidence that future changes won’t break existing functionality. Without it, we’re just hoping everything keeps working.

moticless · 2025-06-23T08:47:51Z

src/expire.c

+    TEST("estoreGetBucket with cluster mode OFF") {
+        server.cluster_enabled = 0; // Non-cluster mode
+
+        estore *es = estoreCreate(&testEbType, 2); // 2^2 = 4 buckets
+        ebuckets *expected = &es->buckets[0];
+
+        for (int i = 0; i < 4; i++) {
+            ebuckets *b = estoreGetBucket(es, i);
+            assert(b == expected); // All slots should return the first bucket in non-cluster mode
+        }
+
+        estoreRelease(es);
+    }


Covering even the trivial test cases helps improve code coverage. Good coverage shows that the code works as expected and is designed properly. It also gives us confidence that future changes won’t break existing functionality. Without it, we’re just hoping everything keeps working.

moticless · 2025-06-23T08:49:41Z

src/expire.c

+    TEST("estoreGetBucket with cluster mode ON") {
+        server.cluster_enabled = 1; // Cluster mode enabled
+
+        estore *es = estoreCreate(&testEbType, 2); // 4 buckets
+
+        for (int i = 0; i < 4; i++) {
+            ebuckets *b = estoreGetBucket(es, i);
+            assert(b == &es->buckets[i]); // Each slot should map to its corresponding bucket
+        }
+
+        estoreRelease(es);
+    }


Better have it covered by TCL test to verify correctness of cluster mode.

Note that there are cluster tests currently in comment at file expire.tcl - We should strive to make it functional again.

If coverage is not sufficient, then we better extend it there.

moticless · 2025-06-23T09:13:20Z

src/expire.c

+    TEST("estoreAdd inserts kvobj at correct address in expected bucket") {
+        server.cluster_enabled = 0;
+
+        estore *es = estoreCreate(&testEbType, 2); // 4 buckets (slots 0-3)
+        TestKVObj *kv = zmalloc(sizeof(TestKVObj));
+        long long expire_time = 1;
+        int slot = 1;
+
+        assert(estoreSize(es) == 0);
+
+        estoreAdd(es, (kvobj*)kv, slot, expire_time);
+        assert(estoreSize(es) == 1);
+
+        // Get the expected bucket
+        ebuckets *bucket = estoreGetBucket(es, slot);
+        EbucketsIterator iter;
+        int found = 0;
+
+        ebStart(&iter, *bucket, es->bucket_type);
+        while (iter.currItem) {
+            if (iter.currItem == kv) {
+                found = 1;
+                break;
+            }
+            ebNext(&iter);
+        }
+        ebStop(&iter);
+
+        assert(found == 1); // kvobj was found by pointer
+
+        zfree(kv);
+        estoreRelease(es);
+    }


This test should also be covered at TCL level and if needed by DEBUG HTSTATS (Maybe we should have dedicate DEBUG EXPIRE-STATS.

moticless · 2025-06-23T09:28:44Z

src/expire.c

+    TEST("estoreRelease with NULL does nothing") {
+        // Should not crash or do anything
+        estoreRelease(NULL);
+    }
+
+    TEST("estoreRelease frees all resources for valid estore") {
+        // Setup
+        estore *es = estoreCreate(&testEbType, 2); // 4 buckets
+
+        // Add an item to ensure some real content exists
+        TestKVObj *kv = zmalloc(sizeof(TestKVObj));
+        estoreAdd(es, (kvobj*)kv, 1, 1);
+
+        // Just ensure it was added (not strictly necessary, but good check)
+        assert(estoreSize(es) == 1);
+
+        // Release the estore
+        estoreRelease(es);
+
+        // We can't assert anything post-release (pointer is now invalid),
+        // but Valgrind or AddressSanitizer will catch use-after-free if any.
+        zfree(kv); // Caller owns kvobj memory separately
+    }


This one should be also tested at TCL level. Add items and then flushdb . Then verify it is empty via statstitcs. Note that i made another commit that fixes flushdb/flushall which takes care to empty estoreRelease() before kvstoreRelease() (This is because ebuckets relies on embedded data in the keys and we should release it before releasing the keyspace)

moticless · 2025-06-23T09:35:54Z

src/expire.c

+    TEST("estoreRemove removes kvobj and decrements count") {
+        server.cluster_enabled = 0;
+
+        // Create estore with 1 bucket
+        estore *es = estoreCreate(&testEbType, 1);
+
+        // Allocate and add a test kvobj
+        TestKVObj *kv = zmalloc(sizeof(TestKVObj));
+        long long expire_time = 1;
+
+        estoreAdd(es, (kvobj*)kv, 0, expire_time);
+        assert(estoreSize(es) == 1);
+
+        // Remove the kvobj
+        estoreRemove(es, 0, (kvobj*)kv);
+        assert(estoreSize(es) == 0);
+
+        // Cleanup
+        zfree(kv);
+        estoreRelease(es);
+    }


I think we should have here few meaningfull tests. Something iterative like:

T = current time + 100 seconds

For precision : P // take care it expands also beyond 1 hour....

Verify ESTORE stat

For i:0->100

Add item with expiration time {T+i * P}

Verify ESTORE stat

For i:0->100

Verify expiration time of item

Remove item

Verify ESTORE stat

moticless reviewed Jun 23, 2025

View reviewed changes

sggeorgiev added 3 commits June 24, 2025 10:19

Add unit tests to estore

893639e

Add ebMemUsage function

71af8d8

Add ebMemUsage function

c3138e7

sggeorgiev force-pushed the redesign-expire branch from e44f440 to c3138e7 Compare June 24, 2025 07:20

sggeorgiev added 2 commits June 24, 2025 13:50

Add logic to calculate AVG TTL

4d08ce6

Add logic to adjust the maxCascade value using a PID controller

b901c04

sggeorgiev force-pushed the redesign-expire branch from 1fcc98e to b901c04 Compare June 25, 2025 11:00

sggeorgiev added 7 commits June 25, 2025 16:18

Add logic to adjust the maxCascade value using a PID controller

bf8c1df

Add logic to adjust the maxCascade value using a PID controller

0257198

Add logic to adjust the maxCascade value using a PID controller

6404c7d

Add metrics tracking item count by bucket level

fe7b941

Added resusable pid controller

b0afedb

Changed active expire to use pid controler

75848d3

Changed active expire to use pid controler

18f029b

sggeorgiev closed this Jul 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add unit tests to estore #1

Add unit tests to estore #1

Uh oh!

sggeorgiev commented Jun 20, 2025

Uh oh!

moticless Jun 23, 2025

Uh oh!

sggeorgiev Jun 23, 2025

Uh oh!

moticless Jun 23, 2025

Uh oh!

sggeorgiev Jun 23, 2025

Uh oh!

moticless Jun 23, 2025

Uh oh!

sggeorgiev Jun 23, 2025

Uh oh!

moticless Jun 23, 2025

Uh oh!

moticless Jun 23, 2025

Uh oh!

moticless Jun 23, 2025

Uh oh!

moticless Jun 23, 2025

Uh oh!

Uh oh!

Add unit tests to estore #1

Add unit tests to estore #1

Uh oh!

Conversation

sggeorgiev commented Jun 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!