stream auto trimming , no need for dedicated thread#88
Conversation
WalkthroughRemoved scheduled Redis stream trimming from RedisStreamEventStore; trimming is now applied during publish calls via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java (1)
384-392: Clean up leftovertrimEveryinBuilderThe constructor and
build()no longer accept a trim interval, butBuilderstill declares aDuration trimEveryfield that is never set or used; consider removing it (and any related comments/docs) so the builder surface matches the actual configuration options.Also applies to: 440-448
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: build (25) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java (2)
98-136: Constructor defaulting forstreamMaxLengthlooks goodUsing a nullable
Integerand normalizingnull/non-positive values toDEFAULT_MAX_LENwith a warning keeps direct construction and the builder behavior aligned; no correctness issues stand out.
180-188: Inline stream trimming on publish changes retention/perf behaviorMoving trimming into
.add(StreamAddArgs.entry(...).trimNonStrict().maxLen(streamMaxLength).noLimit())means every publish now enforcesstreamMaxLengthand may trim immediately; please confirm this matches your intended retention policy and doesn’t introduce unwanted latency at high write rates.
Qodana for JVM45 new problems were found
☁️ View the detailed Qodana report Detected 123 dependenciesThird-party software listThis page lists the third-party software dependencies used in project
Contact Qodana teamContact us at qodana-support@jetbrains.com
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java (1)
239-281: Lifecycle ofrunningflag may prevent reuse after all listeners unsubscribe
pollLoopexits early whenrunningis false (Line 241), andunsubscribe0setsrunningto false oncelistenersbecomes empty (Lines 328‑330). There is no corresponding reset ofrunningtotruewhen new subscribers are added, so if the store is reused after all listeners are removed, newly created pollers will immediately exit and never resume reading.If reuse after “all unsubscribed” is a valid scenario in this project, consider:
- Either not flipping
runningto false inunsubscribe0, and relying solely onshutdown0to stop the store, or- Explicitly setting
runningback to true insubscribe0before (or when)ensurePolleris called.Also applies to: 318-331
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (17) / build
- GitHub Check: build (21) / build
- GitHub Check: build (25) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java (3)
97-138: Constructor defaulting forstreamMaxLengthis consistent and safeThe updated constructor cleanly drops the trim interval and keeps
streamMaxLengthas the single trimming knob, with sensible defaulting when null/≤0 and clear WARN logs. No correctness issues spotted; callers using the Builder will still get the same defaulting behavior.
439-447: Builder wiring to new constructor signature is coherentThe Builder’s
build()now forwardsstreamMaxLenas the last argument to the trimmed constructor, which aligns with the new signature and keeps the defaulting behavior in the constructor. No issues here; call sites using the Builder should compile cleanly and continue to get defaults whenstreamMaxLenis not set.
179-187: Implementation correctly uses Redisson 3.52.0 stream trimming APIThe
StreamAddArgs.entry(...).trimNonStrict().maxLen(streamMaxLength).noLimit()pattern is fully supported in Redisson 3.52.0. This correctly maps to RedisXADD ... MAXLEN ~ streamMaxLength, trimming oldest entries approximately on each publish. The API, behavior, and version constraint are all verified as sound.The non-strict (approximate) trimming trade-off is acceptable for bounded stream management without a dedicated trim thread. If strict enforcement becomes necessary, switching to
trimStrict()is straightforward.
Description
No trimming logic needed
Type of Change
Related Issue
Closes #(issue number)
Changes Made
Testing
mvn testChecklist
Additional Notes
Any additional information, screenshots, or context that reviewers should know.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.