Conversation
WalkthroughThis PR introduces a new RedisStreamEventStore implementation for Redis Streams-based event publishing and subscribing, updates HazelcastEventStore's topic naming logic to consistently prefix topics, adds module exports, and includes multiple integration tests for distributed scenarios with both reliable and stream-based event stores in SINGLE_CHANNEL and MULTI_CHANNEL modes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ 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 |
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 (2)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
32-35: Check if RedissonEventStore import is used.Line 32 imports
RedissonEventStorebut it doesn't appear to be referenced in this test. Consider removing unused imports.#!/bin/bash # Verify if RedissonEventStore is used in this file rg -n 'RedissonEventStore(?!\.Builder)' netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (1)
32-35: Check if RedissonEventStore import is used.Line 32 imports
RedissonEventStorebut it doesn't appear to be referenced in this test. Consider removing unused imports.#!/bin/bash # Verify if RedissonEventStore is used in this file rg -n 'RedissonEventStore(?!\.Builder)' netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java(1 hunks)netty-socketio-core/src/main/java/module-info.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (1)
RedissonStoreFactory(33-75)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java (1)
RedissonReliableEventStore(47-336)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventMessage.java (1)
EventMessage(21-44)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/ListenerRegistration.java (1)
ListenerRegistration(25-43)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java (1)
RedisStreamEventStore(51-506)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.java (5)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.java (1)
TestInstance(36-134)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
TestInstance(38-136)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (1)
TestInstance(38-133)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (1)
RedissonStoreFactory(33-75)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java (1)
RedissonReliableEventStore(47-336)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (1)
RedissonStoreFactory(33-75)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java (1)
RedisStreamEventStore(51-506)
🪛 ast-grep (0.40.0)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.java
[info] 46-46: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
⏰ 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 (16)
netty-socketio-core/src/main/java/module-info.java (1)
20-20: LGTM!The module export for the new redis_stream package follows the established pattern and correctly exposes the RedisStreamEventStore public API.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java (6)
106-182: LGTM!The constructor and initialization logic properly validates required parameters, applies sensible defaults with warning logs, and sets up streams and trimming appropriately based on the EventStoreMode.
202-213: LGTM!The publish logic correctly attaches the nodeId for deduplication and uses computeIfAbsent for lazy stream initialization.
264-306: LGTM!The polling loop correctly handles async reads with timeout, processes records while skipping messages from the same node, and reschedules itself for continuous polling. The empty map check at line 285 protects the subsequent value access.
308-330: LGTM!The dispatch logic correctly uses runtime type checking with
isInstancebefore performing the cast, ensuring type safety when invoking listeners.
366-391: LGTM!The shutdown logic comprehensively cleans up all resources, including pollers, executors, and internal maps, using
shutdownNow()for immediate termination.
423-504: LGTM!The Builder class follows best practices with proper validation in setters (empty prefix check, positive streamMaxLen validation) and a fluent API for configuration.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.java (3)
45-49: LGTM!The
findAvailablePort()utility correctly usesServerSocket(0)to allocate a free port for testing. The static analysis warning about unencrypted sockets is a false positive—this is only for dynamic port allocation, not for actual data transmission.
55-106: LGTM!The test setup properly initializes Redis container, creates Redisson clients, configures two SocketIOServer nodes with RedissonReliableEventStore in MULTI_CHANNEL mode, and registers appropriate event handlers.
114-132: LGTM!The teardown logic correctly cleans up all resources in the proper order: nodes, Redis clients, and the Redis container.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
70-70: LGTM!The test now correctly uses
RedisStreamEventStore.Builderfor MULTI_CHANNEL mode, testing the new Redis Streams-based event store implementation.Also applies to: 92-92
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (1)
69-69: LGTM!The test now correctly uses
RedisStreamEventStore.Builderwith SINGLE_CHANNEL mode, properly testing the new Redis Streams-based event store for single-channel scenarios.Also applies to: 90-90
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.java (3)
46-50: LGTM!The
findAvailablePort()utility correctly usesServerSocket(0)for dynamic port allocation in tests. The static analysis warning is a false positive.
55-105: LGTM!The test setup properly configures two SocketIOServer nodes with RedissonReliableEventStore in SINGLE_CHANNEL mode, with appropriate event handlers for distributed room management testing.
113-131: LGTM!The teardown logic ensures proper cleanup of all resources in the correct order.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (1)
96-101: Remove this comment - no breaking change detected.The topic naming is not a new behavioral change. The codebase consistently applies
topicPrefixacross all event store implementations, with a default prefix"SOCKETIO4J:"provided. This is intentional design with builder configuration support (topicNamePrefix()method). The changelog contains no breaking change notice for topic naming, and all tests use this default behavior without issue.Likely an incorrect or invalid review comment.
Description
Brief description of the changes in this PR.
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.