Active ttl#1
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR restructures a concurrent hash map implementation into a package-organized design with TTL-based expiration. It moves ChangesConcurrent Hash Map with TTL Support and Testing Infrastructure
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (2)
src/main/java/cache/Jmap.java (1)
13-13: ⚡ Quick winEncapsulate
node_ctto protect map invariants.Exposing
node_ctas a public mutableAtomicIntegerlets external callers alter internal state and break resize/count correctness. Preferprivate final+ a read-only accessor.Proposed refactor
- public AtomicInteger node_ct = new AtomicInteger(0); + private final AtomicInteger node_ct = new AtomicInteger(0); + + public int getNodeCount() { + return node_ct.get(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/cache/Jmap.java` at line 13, The node_ct field in the Jmap class is currently exposed as public and mutable, allowing external code to directly modify the internal node count and break map invariants. Change the visibility of node_ct to private, make it final to prevent reassignment, and add a public read-only accessor method (such as getNodeCount) that returns the current value of node_ct using the get() method of AtomicInteger. This ensures external code can only read the node count without modifying internal state.src/test/java/TTLTest.java (1)
20-45: ⚡ Quick winEnsure
TTLManager.shutdown()runs even on failed assertions.If any check fails before method end, the cleanup thread is left running. Wrap each test body with
try/finallyand callttlManager.shutdown()infinally.Also applies to: 50-93, 98-127, 132-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/TTLTest.java` around lines 20 - 45, The tests (e.g., testBasicTTL) currently call ttlManager.shutdown() only at the end, so a failed assertion leaves the TTLManager thread running; wrap the test body that uses TTLManager (instantiate TTLManager map and schedule calls inside the test method) in a try / finally and call ttlManager.shutdown() in the finally block (ensure TTLManager.shutdown() is invoked even on assertion failures); apply the same change to the other test methods referenced (lines 50-93, 98-127, 132-173) so each test guarantees TTLManager.shutdown() is always executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dependency-reduced-pom.xml`:
- Around line 23-27: The two <transformer> entries are missing their required
implementation attributes: the transformer that contains <mainClass> should be
declared as ManifestResourceTransformer
(implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer")
and the empty <transformer/> should be declared as ServicesResourceTransformer
(implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer");
update the <transformer> elements to include these implementation attributes so
Shade will recognize and apply ManifestResourceTransformer and
ServicesResourceTransformer when re-running with the dependency-reduced POM.
In `@README.md`:
- Line 21: Update the README entry that references the TTL test path: replace
the incorrect path `src/test/java/cache/TTLTest.java` with the actual path
`src/test/java/TTLTest.java` so the description and list entry correctly point
to the TTLTest.java unit test; ensure any other occurrences of the old path in
README.md are updated consistently.
- Line 58: In the README's "Future scope" sentence replace the typo "fom" with
"from" so the sentence reads "Might move from one single global lock to sharding
the entire map into smaller pieces..." — update that exact word in the "Future
scope" paragraph to correct the user-facing docs.
- Line 30: The documentation says putexp accepts TTL in seconds but the code
uses raw milliseconds; update the implementation so KVStore and Jmap.putexp
treat the API TTL as seconds by converting seconds to milliseconds before
storing—locate the KVStore.putexp (or KVStore.set/put) code that computes the
expiry and multiply the received ttl_seconds by 1000 (or otherwise convert to
ms) when computing the expiry timestamp, and ensure Jmap.putexp passes the
CLI/API ttl value unchanged (seconds) to KVStore; also add a brief unit comment
near KVStore.putexp to avoid future confusion.
In `@src/main/java/cache/Jmap.java`:
- Around line 99-119: The putexp method currently computes its own expiry
timestamp while callers (TTLManager) compute a separate timestamp, causing
mismatches; modify putexp (or its callers) so a single canonical absolute expiry
is used: either change putexp signature to accept an absoluteExpiry long (and
store that exact value into Jnode.ttl and use it for any return/scheduling), or
have putexp compute the expiry and return the exact stored expiry to the caller
for scheduling; update references (putexp calls, TTLManager queue insertions,
Jnode.ttl assignment) accordingly so the same timestamp is used for both map
storage and TTLManager scheduling.
- Around line 28-31: The Jmap constructor should validate the incoming threshold
to avoid pathological resizing: in the Jmap(float threshold) constructor (and
before assigning resize_threshold), check that threshold is finite and > 0 (e.g.
use Float.isFinite(threshold) && threshold > 0f); if not, throw an
IllegalArgumentException with a clear message. This ensures resize_threshold is
never NaN/Infinity/<=0 and prevents constant resize attempts; adjust any callers
if needed to provide a valid threshold.
In `@src/main/java/cache/JmapBenchmark.java`:
- Around line 23-33: The benchmark is allocating strings inside hot paths by
doing "key"+i and "val"+i; modify JmapBenchmark to precompute and store all key
and value strings in arrays or Lists during setup (use the existing KEY_SPACE
and the setup() method that initializes jmap and chm), populate e.g. String[]
keys and String[] vals and use those arrays in all benchmark operations (all
jmap.get/put and chm.get/put calls) instead of concatenating at call time so the
benchmark reuses the prebuilt keys/values rather than allocating per operation.
In `@src/main/java/cache/KVStore.java`:
- Around line 45-48: loadLog currently only replays put and del/remove entries
so putexp entries are lost on restart; change the logging and recovery to
persist an absolute expiration timestamp (epoch millis) for putexp instead of
the original TTL and update loadLog to parse and replay putexp by restoring the
key with that absolute expiry (using the same symbol LOG_FILE, loadLog, and the
code path that handles put/putexp/del). Ensure the writer that appends putexp
records writes the expiry timestamp field and the parser in loadLog recognizes a
putexp record type and calls the existing put-with-expiry logic (or a new
restorePutWithExpiry method) so the original expiry instant is preserved across
restarts.
- Around line 25-31: main() currently bypasses store startup by commenting out
TTLManager initialization, initializeLog(), loadLog(), and interactiveMode() and
directly calling stressTest(), leaving ttlManager null and breaking putexp;
restore startup sequence in main() by re-enabling TTLManager creation (new
TTLManager<>(map) or appropriate constructor), call initializeLog(), call
loadLog() to replay persisted operations, then invoke interactiveMode() so the
CLI is reachable, and only run stressTest() optionally (e.g., behind a flag)
after the store is initialized; ensure ttlManager is assigned before any code
path that calls putexp().
- Around line 143-147: The code treats TTL as seconds but Jmap.putexp(...)
expects milliseconds and TTLManager.schedule(...) uses exact millisecond
timestamps; convert the parsed seconds to milliseconds and use that same
millisecond value for both the map and scheduler to avoid desync. Specifically,
compute long ttlSeconds = Long.parseLong(parts[3]); long ttlMillis = ttlSeconds
* 1000L; use map.putexp(parts[1], parts[2], ttlMillis); compute expiryTime =
System.currentTimeMillis() + ttlMillis and pass that to
ttlManager.schedule(parts[1], expiryTime); adjust the log to reflect the
original seconds or the millis value consistently.
In `@src/main/java/cache/StressTest.java`:
- Around line 36-38: The test currently computes expiry before calling
map.putexp(...) and then schedules TTLManager.schedule(...) with that
precomputed timestamp, but Jmap (map.putexp) stamps the stored expiry itself
(getExpiry(key) is used for equality checks), so compute and use the actual
expiry that Jmap stored: call map.putexp("key"+key, "val"+key, TTL_MS) first,
then retrieve the stored expiry via map.getExpiry("key"+key) and pass that value
into ttlManager.schedule(...). Apply the same change for the other occurrence at
lines 51-53.
- Around line 65-69: If latch.await(60, TimeUnit.SECONDS) returns false the
current code only calls pool.shutdown() which doesn't interrupt stuck workers;
change the timeout path to call pool.shutdownNow(), then await termination
(e.g., pool.awaitTermination with a short timeout) and ensure the TTL manager is
also stopped there (the same tear-down done in the success path). Update the
code around the latch.await/ pool.shutdown() logic (referencing latch.await,
pool.shutdown, pool.shutdownNow, and the TTL cleanup/TTL_MS sleep) and apply the
same change to the similar block around lines 77-84 so both timeout branches
interrupt workers and stop the TTL manager.
- Around line 46-48: The harness currently treats missing keys during deletes as
errors because a random remove ("map.remove(\"key\" + key)") can legitimately
miss and fall into the outer catch that increments "errors"; update the delete
branch in StressTest (where op < 9) to guard the removal—either check
map.containsKey("key" + key) before calling map.remove(...) or perform the
remove inside a try that only increments "errors" for real failures (not for an
absent-key case)—so benign misses are not counted as harness errors.
In `@src/main/java/cache/TTLManager.java`:
- Around line 27-47: The cleanup loop in TTLManager currently only catches
InterruptedException so any runtime exception from expiryQueue.take(),
map.getExpiry(...), or map.remove(...) will terminate the thread; wrap the
per-iteration processing (the code after expiryQueue.take() returning an
ExpiryEntry) in a broad try-catch(RuntimeException | Error e) that logs the
exception (use the class's logger) and continues the loop so TTL eviction isn't
stopped silently; keep the existing InterruptedException handling to restore
interrupt status and break, and ensure logging includes the key and expiry when
available to aid debugging.
In `@src/test/java/TTLTest.java`:
- Around line 26-29: Tests compute expiry timestamps separately from map.putexp
which causes millisecond skew and makes TTLManager.schedule mismatch
map.getExpiry(key); instead, after calling map.putexp("key", value, ttlMillis)
retrieve the authoritative expiry via map.getExpiry(key) (or use a putexp
overload that returns the expiry) and pass that value into
TTLManager.schedule(...). Update all occurrences for "testkey", key1/key2/key3,
"updatekey", and concurrent keys so schedule() is called with the map-derived
expiry to avoid stale-ignore races with TTLManager.schedule and
TTLManager.getExpiry.
- Around line 9-15: Convert the main-driven TTLTest to proper JUnit tests so CI
runs them: replace the public static void main and the manual calls to
testBasicTTL(), testMultipleTTLs(), testTTLUpdateScenario(),
testConcurrentTTLOperations() with individual `@Test` methods named testBasicTTL,
testMultipleTTLs, testTTLUpdateScenario, testConcurrentTTLOperations in class
TTLTest and switch all Java assert statements to JUnit assertions (e.g.,
org.junit.jupiter.api.Assertions.assertEquals/True) so failures are detected by
Surefire; ensure each test always calls ttlManager.shutdown() by moving shutdown
into a finally block (or using `@AfterEach` to call ttlManager.shutdown()) to
guarantee cleanup on assertion failures; also update pom.xml
maven-surefire-plugin config to enable assertions (-ea / enableAssertions) if
you prefer retaining any assert usage.
---
Nitpick comments:
In `@src/main/java/cache/Jmap.java`:
- Line 13: The node_ct field in the Jmap class is currently exposed as public
and mutable, allowing external code to directly modify the internal node count
and break map invariants. Change the visibility of node_ct to private, make it
final to prevent reassignment, and add a public read-only accessor method (such
as getNodeCount) that returns the current value of node_ct using the get()
method of AtomicInteger. This ensures external code can only read the node count
without modifying internal state.
In `@src/test/java/TTLTest.java`:
- Around line 20-45: The tests (e.g., testBasicTTL) currently call
ttlManager.shutdown() only at the end, so a failed assertion leaves the
TTLManager thread running; wrap the test body that uses TTLManager (instantiate
TTLManager map and schedule calls inside the test method) in a try / finally and
call ttlManager.shutdown() in the finally block (ensure TTLManager.shutdown() is
invoked even on assertion failures); apply the same change to the other test
methods referenced (lines 50-93, 98-127, 132-173) so each test guarantees
TTLManager.shutdown() is always executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0392f3ae-87c2-4426-9eb4-56a95e0af230
📒 Files selected for processing (13)
README.mddependency-reduced-pom.xmlpom.xmlsrc/main/java/Jmap.javasrc/main/java/cache/ExpiryEntry.javasrc/main/java/cache/Jmap.javasrc/main/java/cache/JmapBenchmark.javasrc/main/java/cache/Jnode.javasrc/main/java/cache/KVStore.javasrc/main/java/cache/StressTest.javasrc/main/java/cache/TTLManager.javasrc/main/logs/logs.txtsrc/test/java/TTLTest.java
💤 Files with no reviewable changes (1)
- src/main/java/Jmap.java
Comment on lines
+23
to
+27
| <transformer> | ||
| <mainClass>org.openjdk.jmh.Main</mainClass> | ||
| </transformer> | ||
| <transformer /> | ||
| </transformers> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all shade transformers in dependency-reduced-pom.xml declare implementation classes.
python - <<'PY'
import xml.etree.ElementTree as ET
pom = "dependency-reduced-pom.xml"
ns = {"m": "http://maven.apache.org/POM/4.0.0"}
tree = ET.parse(pom)
root = tree.getroot()
for t in root.findall(".//m:plugin[m:artifactId='maven-shade-plugin']//m:transformer", ns):
print("transformer attributes:", t.attrib, "| has implementation:", "implementation" in t.attrib)
PYRepository: RishabhRawat9/cache
Length of output: 173
🌐 Web query:
Maven Shade Plugin dependency-reduced-pom.xml <transformer> without implementation attribute mainClass ServicesResourceTransformer behavior
💡 Result:
The ServicesResourceTransformer in the Maven Shade Plugin is designed specifically to relocate class names and merge multiple entries within META-INF/services resources [1][2]. It does not have, nor does it support, a mainClass attribute or configuration element [1]. The mainClass configuration parameter is specific to the ManifestResourceTransformer, which is used to define the entry point of an executable JAR by modifying the MANIFEST.MF file [3][4]. Attempting to add a mainClass or manifestEntries element to a ServicesResourceTransformer configuration will result in a build error (often reported as "Cannot find '...' in class") because the underlying Java class for the ServicesResourceTransformer does not contain these properties [5]. Regarding the dependency-reduced-pom.xml, this is a separate feature of the shade goal that automatically generates a simplified POM file for the shaded artifact [6]. Its creation is controlled by the createDependencyReducedPom configuration parameter (which defaults to true) and is independent of the transformers specified in the plugin configuration [6][7]. The transformers themselves do not interfere with or require specific handling for this file [1][6]. To correctly configure these elements: 1. Use ServicesResourceTransformer with no additional attributes within the element [2][8]. 2. Use ManifestResourceTransformer if you need to specify a mainClass [4]. 3. The dependency-reduced-pom.xml is managed automatically by the plugin based on its own configuration parameters, not by the transformers [6].
Citations:
- 1: https://maven.apache.org/plugins/maven-shade-plugin/apidocs/org/apache/maven/plugins/shade/resource/ServicesResourceTransformer.html
- 2: https://maven.apache.org/components/plugins/maven-shade-plugin/examples/resource-transformers.html
- 3: https://maven.apache.org/plugins/maven-shade-plugin/apidocs/org/apache/maven/plugins/shade/resource/ManifestResourceTransformer.html
- 4: https://maven.apache.org/components/plugins/maven-shade-plugin/examples/executable-jar.html
- 5: https://stackoverflow.com/questions/57564196/cannot-find-manifestentries-in-class-org-apache-maven-plugins-shade-resource-s
- 6: https://maven.apache.org/plugins/maven-shade-plugin/shade-mojo.html
- 7: [MSHADE-36] Use reduced dependencies apache/maven-shade-plugin#25
- 8: https://maven.apache.org/plugins/maven-shade-plugin/examples/resource-transformers.html
Fix missing implementation attributes for Shade transformers in dependency-reduced-pom.xml
dependency-reduced-pom.xml (lines 23-27) contains two <transformer> blocks with no implementation class (one includes <mainClass>). Shade transformers are selected/configured via implementation: <mainClass> belongs to ManifestResourceTransformer, and the empty transformer should correspond to ServicesResourceTransformer; as-is the transformer configuration is incomplete/ignored if Shade is ever re-run using this POM.
🔧 Proposed fix
- <transformer>
+ <transformer implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer">
<mainClass>org.openjdk.jmh.Main</mainClass>
</transformer>
- <transformer />
+ <transformer implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <transformer> | |
| <mainClass>org.openjdk.jmh.Main</mainClass> | |
| </transformer> | |
| <transformer /> | |
| </transformers> | |
| <transformer implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer"> | |
| <mainClass>org.openjdk.jmh.Main</mainClass> | |
| </transformer> | |
| <transformer implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"/> | |
| </transformers> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dependency-reduced-pom.xml` around lines 23 - 27, The two <transformer>
entries are missing their required implementation attributes: the transformer
that contains <mainClass> should be declared as ManifestResourceTransformer
(implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer")
and the empty <transformer/> should be declared as ServicesResourceTransformer
(implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer");
update the <transformer> elements to include these implementation attributes so
Shade will recognize and apply ManifestResourceTransformer and
ServicesResourceTransformer when re-running with the dependency-reduced POM.
| - `src/main/java/cache/ExpiryEntry.java`: A supporting class used by `DelayQueue` to track the expiration time of keys. | ||
| - `src/main/java/cache/JmapBenchmark.java`: JMH benchmarking setup to compare throughput running with 1, 8, and 16 concurrent threads. | ||
| - `src/main/java/cache/StressTest.java`: Multi-threaded integration tests running intensive concurrent operations. | ||
| - `src/test/java/cache/TTLTest.java`: Unit tests ensuring correct TTL behavior. |
There was a problem hiding this comment.
TTL test path in project structure appears incorrect.
README lists src/test/java/cache/TTLTest.java, but the provided test snippet is from src/test/java/TTLTest.java. Please correct the path so contributors can find tests reliably.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 21, Update the README entry that references the TTL test
path: replace the incorrect path `src/test/java/cache/TTLTest.java` with the
actual path `src/test/java/TTLTest.java` so the description and list entry
correctly point to the TTLTest.java unit test; ensure any other occurrences of
the old path in README.md are updated consistently.
| ### CLI Commands | ||
|
|
||
| - `put <key> <value>`: Insert or update a key with a value. | ||
| - `putexp <key> <value> <ttl_seconds>`: Insert or update a key with a value and a Time-To-Live (TTL) in seconds. |
There was a problem hiding this comment.
putexp TTL unit is documented as seconds but runtime currently uses raw milliseconds.
The command docs say ttl_seconds, but KVStore and Jmap.putexp currently treat that value as a direct millisecond delta. Please align docs and implementation to one unit (preferably convert seconds → ms in KVStore).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 30, The documentation says putexp accepts TTL in seconds
but the code uses raw milliseconds; update the implementation so KVStore and
Jmap.putexp treat the API TTL as seconds by converting seconds to milliseconds
before storing—locate the KVStore.putexp (or KVStore.set/put) code that computes
the expiry and multiply the received ttl_seconds by 1000 (or otherwise convert
to ms) when computing the expiry timestamp, and ensure Jmap.putexp passes the
CLI/API ttl value unchanged (seconds) to KVStore; also add a brief unit comment
near KVStore.putexp to avoid future confusion.
| Global ReentrantReadWriteLock became a bottleneck as thread count increased. Although multiple readers can hold the read lock concurrently, acquiring and releasing the read lock is not free. Internally, the lock maintains shared state (such as reader counts) that is updated atomically. This lock state resides in a cache line that may be cached by multiple CPU cores. When a thread acquires or releases the read lock, its core must obtain exclusive ownership of that cache line before modifying it. This causes cache coherence traffic as ownership of the cache line moves between cores. Under high read concurrency, many threads contend on the same lock metadata, causing frequent cache-line bouncing and reducing scalability. As thread count increases, the overhead of maintaining the shared reader count becomes significant even though the protected data itself is only being read. | ||
| Future scope: | ||
|
|
||
| Might move fom one single global lock to sharding the entire map into smaller pieces and having locks for those smaller shards because stampedlock doesn't always try to acquire a lock but it can also still fail when no. of writes are more. No newline at end of file |
There was a problem hiding this comment.
Fix typo in “Future scope” sentence (fom → from).
Small docs typo in a user-facing section.
🧰 Tools
🪛 LanguageTool
[grammar] ~58-~58: Ensure spelling is correct
Context: ...y being read. Future scope: Might move fom one single global lock to sharding the ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 58, In the README's "Future scope" sentence replace the
typo "fom" with "from" so the sentence reads "Might move from one single global
lock to sharding the entire map into smaller pieces..." — update that exact word
in the "Future scope" paragraph to correct the user-facing docs.
Source: Linters/SAST tools
Comment on lines
+28
to
+31
| public Jmap(float threshold) { | ||
| this.table = (Jnode<K, V>[]) new Jnode[size.get()]; | ||
| this.resize_threshold = threshold; | ||
| this.rwLocks = new ReentrantReadWriteLock[size.get()]; |
There was a problem hiding this comment.
Validate threshold to prevent pathological resizing behavior.
The constructor accepts invalid values (e.g., <= 0, NaN, Infinity), which can force constant resize attempts and destabilize runtime behavior.
Proposed fix
public Jmap(float threshold) {
+ if (!(threshold > 0.0f) || Float.isNaN(threshold) || Float.isInfinite(threshold)) {
+ throw new IllegalArgumentException("threshold must be a finite value > 0");
+ }
this.table = (Jnode<K, V>[]) new Jnode[size.get()];
this.resize_threshold = threshold;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/cache/Jmap.java` around lines 28 - 31, The Jmap constructor
should validate the incoming threshold to avoid pathological resizing: in the
Jmap(float threshold) constructor (and before assigning resize_threshold), check
that threshold is finite and > 0 (e.g. use Float.isFinite(threshold) &&
threshold > 0f); if not, throw an IllegalArgumentException with a clear message.
This ensures resize_threshold is never NaN/Infinity/<=0 and prevents constant
resize attempts; adjust any callers if needed to provide a valid threshold.
Comment on lines
+46
to
+48
| } else if (op < 9) { | ||
| // 10% remove | ||
| map.remove("key" + key); |
There was a problem hiding this comment.
Don't count absent-key deletes as harness errors.
This workload removes random keys from a shared key space, so misses are expected. Both TTLManager and KVStore.loadLog() already guard map.remove(...) for that reason. Here a benign miss will fall into the outer catch, increment errors, and fail the run.
Suggested fix
} else if (op < 9) {
// 10% remove
- map.remove("key" + key);
+ try {
+ map.remove("key" + key);
+ } catch (IllegalArgumentException ignored) {
+ // key was already absent
+ }
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (op < 9) { | |
| // 10% remove | |
| map.remove("key" + key); | |
| } else if (op < 9) { | |
| // 10% remove | |
| try { | |
| map.remove("key" + key); | |
| } catch (IllegalArgumentException ignored) { | |
| // key was already absent | |
| } | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/cache/StressTest.java` around lines 46 - 48, The harness
currently treats missing keys during deletes as errors because a random remove
("map.remove(\"key\" + key)") can legitimately miss and fall into the outer
catch that increments "errors"; update the delete branch in StressTest (where op
< 9) to guard the removal—either check map.containsKey("key" + key) before
calling map.remove(...) or perform the remove inside a try that only increments
"errors" for real failures (not for an absent-key case)—so benign misses are not
counted as harness errors.
Comment on lines
+65
to
+69
| boolean completed = latch.await(60, TimeUnit.SECONDS); | ||
| pool.shutdown(); | ||
|
|
||
| // let TTL cleanup finish | ||
| Thread.sleep(TTL_MS * 3); |
There was a problem hiding this comment.
The timeout path never tears down stuck workers.
If Line 65 returns false, pool.shutdown() only stops new submissions. Any deadlocked worker keeps the executor alive, so the process can hang after printing DEADLOCK. Escalate to shutdownNow() and wait for termination on the timeout path; this is also the right place to shut down the TTL manager.
Suggested fix
- boolean completed = latch.await(60, TimeUnit.SECONDS);
- pool.shutdown();
+ boolean completed = latch.await(60, TimeUnit.SECONDS);
+ if (!completed) {
+ pool.shutdownNow();
+ pool.awaitTermination(5, TimeUnit.SECONDS);
+ } else {
+ pool.shutdown();
+ }
// let TTL cleanup finish
Thread.sleep(TTL_MS * 3);
+ ttlManager.shutdown();Also applies to: 77-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/cache/StressTest.java` around lines 65 - 69, If latch.await(60,
TimeUnit.SECONDS) returns false the current code only calls pool.shutdown()
which doesn't interrupt stuck workers; change the timeout path to call
pool.shutdownNow(), then await termination (e.g., pool.awaitTermination with a
short timeout) and ensure the TTL manager is also stopped there (the same
tear-down done in the success path). Update the code around the latch.await/
pool.shutdown() logic (referencing latch.await, pool.shutdown, pool.shutdownNow,
and the TTL cleanup/TTL_MS sleep) and apply the same change to the similar block
around lines 77-84 so both timeout branches interrupt workers and stop the TTL
manager.
Comment on lines
+27
to
+47
| try { | ||
| // Blocks until an entry is ready to expire | ||
| ExpiryEntry<K> entry = expiryQueue.take(); | ||
|
|
||
| // key might have been updated/deleted) | ||
| Long currentExpiry = map.getExpiry(entry.getKey()); | ||
| if (currentExpiry != null && currentExpiry == entry.getExpiryTime()) { | ||
| try { | ||
| map.remove(entry.getKey()); | ||
| // System.out.println("[TTL] Expired and removed key: " + entry.getKey()); | ||
| } catch (IllegalArgumentException e) { | ||
| // Key was already removed, ignore | ||
| } | ||
| } | ||
| // If expiry doesn't match, the key was updated — ignore stale entry | ||
|
|
||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Harden cleanup loop against unchecked exceptions.
If map.getExpiry(...) or map.remove(...) throws a runtime exception, the cleanup thread exits and active TTL eviction stops silently.
Proposed fix
while (running) {
try {
// Blocks until an entry is ready to expire
ExpiryEntry<K> entry = expiryQueue.take();
@@
- } catch (InterruptedException e) {
+ } catch (InterruptedException e) {
Thread.currentThread().interrupt();
break;
+ } catch (RuntimeException e) {
+ // keep cleanup thread alive; consider structured logging here
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/cache/TTLManager.java` around lines 27 - 47, The cleanup loop
in TTLManager currently only catches InterruptedException so any runtime
exception from expiryQueue.take(), map.getExpiry(...), or map.remove(...) will
terminate the thread; wrap the per-iteration processing (the code after
expiryQueue.take() returning an ExpiryEntry) in a broad
try-catch(RuntimeException | Error e) that logs the exception (use the class's
logger) and continues the loop so TTL eviction isn't stopped silently; keep the
existing InterruptedException handling to restore interrupt status and break,
and ensure logging includes the key and expiry when available to aid debugging.
Comment on lines
+9
to
+15
| public static void main(String[] args) throws InterruptedException { | ||
| testBasicTTL(); | ||
| testMultipleTTLs(); | ||
| testTTLUpdateScenario(); | ||
| testConcurrentTTLOperations(); | ||
| System.out.println("All TTL tests passed!"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check test-framework wiring and assertion enablement config:"
fd -i 'pom.xml' .
fd -i 'build.gradle' .
fd -i 'build.gradle.kts' .
echo
echo "Maven/Gradle flags related to assertions or test runtime:"
rg -n --iglob 'pom.xml' 'maven-surefire-plugin|argLine|enableAssertions|-ea'
rg -n --iglob 'build.gradle*' 'test\s*\{|enableAssertions|jvmArgs|"-ea"|'\''-ea'\'''
echo
echo "TTLTest structure and assertion usage:"
rg -n 'class TTLTest|public static void main|assert\s' src/test/java/TTLTest.java
rg -n '`@Test`|`@ParameterizedTest`|extends\s+TestCase' src/test/java/TTLTest.javaRepository: RishabhRawat9/cache
Length of output: 463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate TTLTest + its assertion usage ==="
ls -la src/test/java || true
rg -n "class TTLTest|public static void main|assert\s" src/test/java/TTLTest.java || true
echo
echo "=== Show TTLTest methods (limited) ==="
wc -l src/test/java/TTLTest.java
sed -n '1,220p' src/test/java/TTLTest.java
echo
echo "=== Check for any CI/build config invoking TTLTest.main ==="
rg -n "TTLTest" pom.xml src/test/java -S || true
echo
echo "=== Inspect Maven Surefire configuration for argLine / enableAssertions ==="
sed -n '1,220p' pom.xml
rg -n "maven-surefire-plugin|argLine|enableAssertions|-ea|jvmArgs|surefire" pom.xml || true
echo
echo "=== Inspect other possible runners (failsafe/exec/surefire includes patterns) ==="
rg -n "maven-failsafe-plugin|exec-maven-plugin|maven-surefire-plugin|includes|testClassesDirectory|systemPropertyVariables" pom.xml || trueRepository: RishabhRawat9/cache
Length of output: 12423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find CI workflows ==="
fd -a "workflows" . -H 2>/dev/null || true
fd -H ".github/workflows" .github || true
fd -t f ".github/workflows/*" .github 2>/dev/null || true
echo
echo "=== Search for TTLTest / assertions / mvn test invocations in CI files ==="
rg -n "TTLTest|-ea|enableAssertions|maven-surefire-plugin|mvn (test|verify)|gradle test" .github . || true
echo
echo "=== Check if any Maven profile configures -ea or custom test execution ==="
rg -n "profile|enableAssertions|argLine|-ea|surefire" pom.xml || trueRepository: RishabhRawat9/cache
Length of output: 781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Root files ==="
ls -la
echo
echo "=== README section referencing TTLTest ==="
sed -n '1,120p' README.md
echo
echo "=== Search whole repo for TTLTest mentions ==="
rg -n "TTLTest" . || true
echo
echo "=== Inspect pom.xml around maven-surefire-plugin ==="
perl -ne 'print if $.>=70 && $.<=120' pom.xml
echo
echo "=== Inspect dependency-reduced-pom.xml around surefire ==="
perl -ne 'print if $.>=35 && $.<=85' dependency-reduced-pom.xmlRepository: RishabhRawat9/cache
Length of output: 7865
Ensure TTLTest actually runs/asserts in CI (and always shuts down TTLManager)
src/test/java/TTLTest.javais amain-driven harness using Javaassertand contains no JUnit@Testmethods, so Maven Surefire won’t execute any TTL validations (andmainwon’t be called).pom.xmlconfiguresmaven-surefire-pluginbut does not enable assertions (-ea/enableAssertions), so even ifmainis invoked elsewhere, theassert ...checks can be skipped while still printing “All TTL tests passed!”.- Each test method calls
ttlManager.shutdown()only at the end; if an assertion fails, shutdown won’t run—wrap shutdown infinally.
Also applies to assertion lines: 31-32, 36-37, 41-42, 70-71, 75-77, 82-85, 89-90, 117-124, 169-170.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/TTLTest.java` around lines 9 - 15, Convert the main-driven
TTLTest to proper JUnit tests so CI runs them: replace the public static void
main and the manual calls to testBasicTTL(), testMultipleTTLs(),
testTTLUpdateScenario(), testConcurrentTTLOperations() with individual `@Test`
methods named testBasicTTL, testMultipleTTLs, testTTLUpdateScenario,
testConcurrentTTLOperations in class TTLTest and switch all Java assert
statements to JUnit assertions (e.g.,
org.junit.jupiter.api.Assertions.assertEquals/True) so failures are detected by
Surefire; ensure each test always calls ttlManager.shutdown() by moving shutdown
into a finally block (or using `@AfterEach` to call ttlManager.shutdown()) to
guarantee cleanup on assertion failures; also update pom.xml
maven-surefire-plugin config to enable assertions (-ea / enableAssertions) if
you prefer retaining any assert usage.
Comment on lines
+26
to
+29
| long expiryTime = System.currentTimeMillis() + 2000; | ||
| map.putexp("testkey", "testvalue", 2000); | ||
| ttlManager.schedule("testkey", expiryTime); | ||
|
|
There was a problem hiding this comment.
Expiry timestamps are sampled inconsistently with putexp, weakening active-TTL validation.
TTLManager removes only when queued expiry exactly matches map.getExpiry(key) (see src/main/java/cache/TTLManager.java:24-47). Here, schedule times are computed independently from putexp, so millisecond skew can route entries to the “stale, ignore” path and still pass because Jmap.get() lazily returns null for expired keys.
Suggested fix pattern
- long expiryTime = System.currentTimeMillis() + 2000;
map.putexp("testkey", "testvalue", 2000);
- ttlManager.schedule("testkey", expiryTime);
+ Long expiryTime = map.getExpiry("testkey");
+ if (expiryTime != null) {
+ ttlManager.schedule("testkey", expiryTime);
+ }Apply the same pattern for key1/key2/key3, updatekey, and concurrent keys.
Also applies to: 55-67, 111-114, 145-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/TTLTest.java` around lines 26 - 29, Tests compute expiry
timestamps separately from map.putexp which causes millisecond skew and makes
TTLManager.schedule mismatch map.getExpiry(key); instead, after calling
map.putexp("key", value, ttlMillis) retrieve the authoritative expiry via
map.getExpiry(key) (or use a putexp overload that returns the expiry) and pass
that value into TTLManager.schedule(...). Update all occurrences for "testkey",
key1/key2/key3, "updatekey", and concurrent keys so schedule() is called with
the map-derived expiry to avoid stale-ignore races with TTLManager.schedule and
TTLManager.getExpiry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
added a background thread which cleans up stale entries from the store
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores