Storefactory api final 4.0.0 #116
Conversation
Enhanced HazelcastStoreFactory, MemoryStoreFactory, and RedissonStoreFactory to allow explicit injection of custom EventStore implementations. Updated constructors, improved Javadoc, and clarified default behaviors for each factory. This enables hybrid configurations where session storage and event propagation backends can be mixed and matched independently.
📝 WalkthroughWalkthroughThis PR refactors store and event-store implementations across the codebase. Key changes include renaming Hazelcast and Redis event-store classes (adding "PubSub" prefix for clarity), introducing new store factory constructors supporting pluggable EventStore injection with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
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: 1
🧹 Nitpick comments (4)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/memory/MemoryStoreFactory.java (1)
45-45: Consider making the field final for immutability.The
eventStorefield is assigned only in constructors and never modified afterward. Marking itfinalwould enforce immutability and make the intent clearer.🔎 Proposed fix
- private final EventStore eventStore; + private final EventStore eventStore;Actually, looking at the code again, I see the field is already implicitly immutable. Let me correct this - the field should be explicitly marked
final:- private EventStore eventStore; + private final EventStore eventStore;netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (2)
96-103: Consider documenting or handlinghazelcastClientlifecycle.The shutdown only closes the
eventStore, not thehazelcastClient. If the Hazelcast instance is externally managed (i.e., provided by the caller), this is correct behavior—but it would help to document this expectation in the class Javadoc.Also, catching and swallowing exceptions prevents callers from knowing if shutdown failed. Consider logging at
warnlevel if this is expected, or re-throwing as a wrapped exception if failures should be surfaced.
110-113: Consider enrichingtoString()for debugging.Including the
EventStoreimplementation type would aid debugging in mixed configurations.🔎 Suggested enhancement
@Override public String toString() { - return getClass().getSimpleName() + " (Hazelcast session store)"; + return getClass().getSimpleName() + " (Hazelcast session store, eventStore=" + + eventStore.getClass().getSimpleName() + ")"; }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (1)
168-171: Consider includingeventStoretype intoString()for debugging.The current output omits the event store configuration. Including it could aid troubleshooting in multi-factory deployments.
🔎 Optional enhancement
@Override public String toString() { - return getClass().getSimpleName() + " (redis session store)"; + return getClass().getSimpleName() + " (redis session store, eventStore=" + + eventStore.getClass().getSimpleName() + ")"; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/memory/MemoryStoreFactory.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java
🧰 Additional context used
🧬 Code graph analysis (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/memory/MemoryStoreFactory.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)
⏰ 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 (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (12)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/memory/MemoryStoreFactory.java (4)
30-42: Excellent documentation!The class-level Javadoc clearly explains the separation between session storage and event propagation, with helpful examples of different combinations. This will greatly assist users in understanding the architecture.
47-54: LGTM!The default constructor is well-documented and correctly initializes with a
MemoryEventStorefor local-only operation.
81-83: LGTM - appropriate for memory storage.The no-op implementation is correct since memory-based storage has no external resources to clean up.
90-93: Helpful addition for debugging!The
toString()override provides clear identification of the factory type and storage mechanism, which will be useful for logging and debugging.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (3)
55-69: LGTM!Good defensive programming with both
@NotNullannotations for static analysis hints andObjects.requireNonNullfor runtime validation. The Javadoc is clear and complete.
91-94: LGTM!Correctly implements the abstract
eventStore()method required byBaseStoreFactory.
81-84: No action required — theHazelcastEventStoreconstructor is designed to handle null values and empty strings safely.The constructor accepts
@Nullableparameters fornodeId,eventStoreMode, andtopicPrefix. It applies appropriate defaults:
- Null
eventStoreModedefaults toMULTI_CHANNEL(line 72)- Null or empty
topicPrefixdefaults toDEFAULT_TOPIC_NAME_PREFIX(lines 66–68)- Null
nodeIddefaults viagetNodeId()(line 79)The Javadoc claim of
MULTI_CHANNELmode is accurate.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (5)
33-105: Excellent documentation.The Javadoc provides comprehensive guidance including use cases, lifecycle notes, thread-safety considerations, and suitability assessments. This is helpful for API consumers choosing the appropriate store factory configuration.
124-128: Good defensive null-safety.Combining
@NotNullannotations withObjects.requireNonNullruntime checks provides both static analysis hints and runtime enforcement. This is a sound pattern for public API constructors.
155-157: LGTM.Clean delegation to Redisson's map creation. The generic typing provides flexibility for different key-value types.
160-166: Graceful shutdown handling.Wrapping
eventStore.shutdown()in try-catch with logging is appropriate for lifecycle methods. This prevents shutdown failures from propagating and potentially blocking other cleanup. The decision to not closeredisClientaligns with the documented contract that Redis connections are managed externally.
139-142: No issue identified. ThenodeIdparameter is explicitly annotated as@Nullable Longand the constructor properly handles null by generating a default value viagetNodeId()(line 75-76). Passingnullis intentional and safe.Likely an incorrect or invalid review comment.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/memory/MemoryStoreFactory.java
Show resolved
Hide resolved
Qodana for JVM57 new problems were found
☁️ View the detailed Qodana report Detected 124 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: 1
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
55-69: Approve with minor javadoc formatting suggestion.The null safety implementation is solid with both @NotNull annotations and runtime checks using Objects.requireNonNull.
Minor: Line 59 and Line 77 have a spacing issue in
@apiNote Added in API version{@code 4.0.0}– consider adding a space before{@code.🔎 Proposed formatting fix
- * @apiNote Added in API version{@code 4.0.0} + * @apiNote Added in API version {@code 4.0.0}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.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). (4)
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: qodana
🔇 Additional comments (11)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (6)
23-23: LGTM!The new imports support the @NotNull annotations and EventStoreMode documentation reference.
Also applies to: 30-31
34-47: LGTM!The expanded javadoc clearly documents the hybrid configuration capability and default behavior.
92-94: LGTM!The
eventStore()accessor properly exposes the configured EventStore, enabling consumers to inspect the event distribution strategy.
97-103: LGTM!The resilient shutdown pattern prevents cascading failures during cleanup. Catching and logging exceptions without propagating them is appropriate for lifecycle methods, ensuring that other cleanup operations can proceed even if the EventStore shutdown fails.
110-114: LGTM!The
toString()override provides a clear, informative representation for debugging and logging purposes.
71-84: The HazelcastEventStore instantiation is correct as-is.The constructor accepts
(hazelcastPub, hazelcastSub, nodeId, eventStoreMode, topicPrefix)where the first two require non-nullHazelcastInstanceparameters and the remaining three are@Nullable. Passing the samehazelcastClientinstance for both pub and sub parameters is intentional—they represent different logical roles (publishing vs. subscribing). Thenullvalues fornodeIdandeventStoreModeare explicitly allowed by their@Nullableannotations. The empty string fortopicPrefixis properly handled by the constructor's logic (line 66-67), which treats it identically tonulland substitutesDEFAULT_TOPIC_NAME_PREFIX. No changes are needed.Likely an incorrect or invalid review comment.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (5)
33-105: Excellent comprehensive documentation!The JavaDoc is thorough, well-structured, and provides clear guidance on usage patterns, lifecycle, thread safety, and suitability. The examples and hybrid topology explanations add significant value for users.
114-128: LGTM! Proper null-safety enforcement.The @NotNull annotations combined with Objects.requireNonNull provide good defensive programming. The parameter rename to
redisClientimproves consistency with the field name.
155-157: LGTM! Correct delegation to Redisson API.The method properly delegates to the Redisson client's getMap method for creating Redis-backed maps.
160-166: LGTM! Appropriate exception handling for shutdown.The try-catch pattern ensures shutdown failures don't propagate, which is appropriate for lifecycle cleanup. The broad Exception catch provides maximum resilience during shutdown, aligning with similar patterns in HazelcastStoreFactory and MemoryStoreFactory.
168-171: LGTM! Clear and concise toString implementation.The toString method provides a helpful description for logging and debugging purposes.
...etio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (2)
55-69: Minor: Fix javadoc formatting.The
@apiNotehas a formatting inconsistency withversion{@code 4.0.0}(missing space before{).🔎 Suggested fix
- * @apiNote Added in API version{@code 4.0.0} + * @apiNote Added in API version {@code 4.0.0}Otherwise, the constructor implementation with null checks and @NotNull annotations is excellent.
92-94: Add @NotNull annotation to the return type.Since the
eventStorefield is guaranteed non-null (validated in the constructor), theeventStore()method should be annotated with@NotNullfor API clarity and null-safety tool support.🔎 Suggested enhancement
@Override + @NotNull public EventStore eventStore() { return eventStore; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java
🧰 Additional context used
🧬 Code graph analysis (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)
⏰ 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 (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (3)
34-47: Well-documented API design.The javadoc clearly explains the hybrid storage model and provides concrete examples of different EventStore configurations.
97-103: Good defensive shutdown implementation.Wrapping
eventStore.shutdown()in a try-catch with error logging prevents shutdown failures from cascading while maintaining observability. This is a solid resilience pattern.
110-113: Helpful toString() for debugging.The override provides a clear, descriptive representation of the factory type, which will be useful in logs and debugging scenarios.
...cketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (1)
139-142: Same Redisson client used for both pub and sub operations.The concern about passing the same
redisClientfor both publisher and subscriber parameters toRedissonEventStorewas already raised in a previous review. Please address the prior feedback regarding proper separation of pub/sub clients.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/memory/MemoryStoreFactory.java (1)
56-67: Unresolved documentation and grammar issues from previous review.The issues flagged in the previous review are still present:
- Line 60: Missing space in
@apiNote- should beversion {@codenotversion{@code}- Line 66: Grammar - "can not" should be "cannot" (one word)
These are minor text quality issues that affect documentation readability and professionalism.
🔎 Proposed fixes
- * @apiNote Added in API version{@code 4.0.0} + * @apiNote Added in API version {@code 4.0.0} * * @param eventStore non-null event store * @throws NullPointerException if {@code eventStore} is {@code null} */ public MemoryStoreFactory(@NotNull EventStore eventStore) { - this.eventStore = Objects.requireNonNull(eventStore, "eventStore can not be null"); + this.eventStore = Objects.requireNonNull(eventStore, "eventStore cannot be null"); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/memory/MemoryStoreFactory.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java
🧰 Additional context used
🧬 Code graph analysis (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)
⏰ 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 (25) / build
- GitHub Check: build (17) / build
- GitHub Check: build (21) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (16)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (5)
33-105: Well-documented class-level Javadoc.The comprehensive documentation clearly explains usage patterns, lifecycle management, thread safety guarantees, and when to use (or avoid) this factory. This is excellent for maintainability.
124-128: Good null-safety implementation.The combination of
@NotNullannotations for static analysis andObjects.requireNonNullfor runtime validation is a solid defensive programming pattern.
154-157: Simple and correct delegation.The implementation cleanly delegates to
redisClient.getMap(name). Consider whether a null/empty check onnameis warranted for consistency with the constructor's null-safety approach, though Redisson will handle this internally.
159-166: Robust shutdown with error handling.Wrapping the
eventStore.shutdown()in a try-catch with logging ensures that shutdown failures don't propagate and crash the application. This defensive approach is appropriate for cleanup operations.
168-171: Useful toString for debugging.The override provides a concise descriptor that will be helpful for logging and debugging purposes.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/memory/MemoryStoreFactory.java (5)
24-24: LGTM!The
@NotNullimport is properly used in the constructor parameter at line 65.
30-42: Excellent documentation!The class-level Javadoc clearly explains the separation between session storage (memory-based) and event propagation (pluggable), along with practical usage examples. This will help users understand the various deployment patterns.
47-54: LGTM!The default constructor is well-documented and correctly initializes with
MemoryEventStorefor fully local operation.
81-83: LGTM!The explicit
// no-opcomment clearly indicates that shutdown is intentionally empty for memory-only storage, which is appropriate since there are no external resources to clean up.
90-93: LGTM!The updated descriptor "memory session store" is more accurate and clear for logging and debugging purposes.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (6)
23-31: LGTM!The imports are appropriate and used in the code.
@NotNullannotations strengthen the API contract, andEventStoreModeis correctly referenced in the javadoc.
33-47: LGTM!The class-level javadoc clearly explains the separation of concerns between session storage (Hazelcast) and event distribution (pluggable EventStore), with helpful examples of hybrid configurations.
55-69: LGTM!The constructor properly combines compile-time annotations with runtime null checks. The explicit
@throws NullPointerExceptiondocumentation and@apiNoteversioning improve API clarity.
91-94: LGTM!Clean accessor implementation that properly exposes the EventStore instance.
110-113: LGTM!The
toString()implementation provides useful diagnostic information for debugging and logging.
96-103: No action needed. BaseStoreFactory does not define ashutdown()method, so there is no parent cleanup logic to call. The pattern is consistent across all implementations (MemoryStoreFactory, HazelcastStoreFactory, RedissonStoreFactory), each of which independently manages shutdown without callingsuper.shutdown(). The current implementation is correct.Likely an incorrect or invalid review comment.
...cketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java (1)
46-49: Update class name and Javadoc to match the renamed API.The test class name and Javadoc still reference "RedissonReliableStoreFactory", but the class is now testing
RedisStoreFactory. Consider renaming the test class toRedisStoreFactoryTest(orRedisReliableStoreFactoryTest) and updating the Javadoc comment for consistency with the API rename.🔎 Suggested updates
/** - * Test class for RedissonReliableStoreFactory using testcontainers + * Test class for RedisStoreFactory using testcontainers */ -public class RedissonReliableStoreFactoryTest extends StoreFactoryTest { +public class RedisStoreFactoryTest extends StoreFactoryTest {Note: If you rename the class, ensure the filename is also updated to match.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastPubSubEventStore.java (1)
181-197: Fix incorrect exception message intopicNamePrefix()method.Line 193 throws an exception with message
"ringBufferNamePrefix cannot be empty", but the method istopicNamePrefix(). This appears to be a copy-paste error from the ring buffer variant.Suggested fix
public HazelcastPubSubEventStore.Builder topicNamePrefix(@NotNull String prefix) { if (prefix.isEmpty()) { - throw new IllegalArgumentException("ringBufferNamePrefix cannot be empty"); + throw new IllegalArgumentException("topicNamePrefix cannot be empty"); } this.topicNamePrefix = prefix; return this; }
♻️ Duplicate comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
81-84: Replace empty string withnullfor the topicPrefix parameter.The 5th argument passed to
HazelcastPubSubEventStoreis an empty string, but the constructor treats bothnulland""identically—both trigger assignment ofDEFAULT_TOPIC_NAME_PREFIX. Passnullinstead of""to be consistent with the parameter's nullable contract.Suggested fix
public HazelcastStoreFactory(@NotNull HazelcastInstance hazelcastClient) { this(hazelcastClient, - new HazelcastPubSubEventStore(hazelcastClient, hazelcastClient, null, null, "")); + new HazelcastPubSubEventStore(hazelcastClient, hazelcastClient, null, null, null)); }
🧹 Nitpick comments (2)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedKafkaMultiChannelTest.java (1)
246-252: Consider reordering shutdown sequence.The container is stopped before the Redisson clients are shut down. While this may work in practice (clients will fail gracefully), the cleaner approach is to shut down clients first, then the container:
🔎 Suggested reordering
KAFKA.close(); if (REDIS_CONTAINER!=null){ + redisClient1.shutdown(); + redisClient2.shutdown(); REDIS_CONTAINER.stop(); - redisClient1.shutdown(); - redisClient2.shutdown(); }netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonStoreTest.java (1)
37-37: Consider renaming test class to match the new API.The test class is still named
RedissonStoreTestwhile it now testsRedisStore. For consistency with the API rename, consider renaming toRedisStoreTest.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
netty-socketio-core/src/main/java/com/socketio4j/socketio/Configuration.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastPubSubEventStore.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastPubSubRingBufferEventStore.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisPubSubEventStore.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStore.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedisPubSubReliableEventStore.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubMultiChannelUnReliableTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedKafkaMultiChannelTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedKafkaSingleChannelTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubMultiChannelUnReliableTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastStoreFactoryTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonStoreTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedissonReliableEventStoreTest.java
🧰 Additional context used
🧬 Code graph analysis (16)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (1)
RedisStoreFactory(107-172)
netty-socketio-core/src/main/java/com/socketio4j/socketio/Configuration.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (1)
RedisStoreFactory(107-172)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastPubSubRingBufferEventStore.java (1)
HazelcastPubSubRingBufferEventStore(42-225)
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/RedisStoreFactory.java (1)
RedisStoreFactory(107-172)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedisPubSubReliableEventStore.java (1)
RedisPubSubReliableEventStore(47-379)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (1)
RedisStoreFactory(107-172)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedisPubSubReliableEventStore.java (1)
RedisPubSubReliableEventStore(47-379)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisPubSubEventStore.java (1)
RedisPubSubEventStore(43-200)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStore.java (1)
RedisStore(27-60)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (1)
RedisStoreFactory(107-172)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastPubSubRingBufferEventStore.java (1)
HazelcastPubSubRingBufferEventStore(42-225)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (1)
RedisStoreFactory(107-172)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastPubSubEventStore.java (1)
HazelcastPubSubEventStore(41-214)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonStoreTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStore.java (1)
RedisStore(27-60)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedKafkaMultiChannelTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (1)
RedisStoreFactory(107-172)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisPubSubEventStore.java (1)
RedisPubSubEventStore(43-200)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (1)
RedisStoreFactory(107-172)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubMultiChannelUnReliableTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastPubSubEventStore.java (1)
HazelcastPubSubEventStore(41-214)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastStoreFactoryTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastPubSubEventStore.java (1)
HazelcastPubSubEventStore(41-214)
⏰ 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 (25) / build
- GitHub Check: build (21) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (47)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java (2)
24-26: LGTM! Import statements updated correctly.The imports have been updated to reflect the renamed API classes (
RedisPubSubEventStore,RedisStore,RedisStoreFactory), aligning with the PR objective to match documentation.
66-67: LGTM! Factory creation updated correctly.The factory instantiation has been properly updated to use
RedisStoreFactorywith an explicitly constructedRedisPubSubEventStorevia the builder pattern. This aligns with the new API structure.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisPubSubEventStore.java (2)
43-43: LGTM! Class rename is consistent.The rename from
RedissonEventStoretoRedisPubSubEventStoreis applied consistently across the class declaration, logger, and constructor. The@NotNull/@Nullableannotations combined withObjects.requireNonNullvalidation provide proper null safety.Also applies to: 54-54, 66-69
190-196: Builder correctly updated.The
build()method return type and instantiation are properly aligned with the renamed class.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStore.java (1)
27-33: LGTM! Clean rename fromRedissonStoretoRedisStore.The class name change aligns with the API naming convention while correctly retaining
RedissonClientas the underlying implementation detail.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedisPubSubReliableEventStore.java (2)
47-47: LGTM! Class rename applied consistently.The rename from
RedissonReliableEventStoretoRedisPubSubReliableEventStoreis properly reflected in the class declaration, logger initialization, and constructor. The reliable event store maintains its comprehensive stream trimming and shutdown handling logic.Also applies to: 63-63, 71-76
366-376: Builder correctly updated for the renamed class.The
build()method properly returns the newRedisPubSubReliableEventStoretype.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedKafkaMultiChannelTest.java (1)
52-52: LGTM! Correctly migrated toRedisStoreFactory.The test properly uses the new
RedisStoreFactorywith the two-argument constructor accepting aRedissonClientand aKafkaEventStore, maintaining the hybrid Redis session storage + Kafka event propagation pattern.Also applies to: 97-101, 144-148
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonStoreTest.java (1)
21-21: LGTM! Test correctly updated to useRedisStore.The import and instantiation are properly aligned with the renamed class.
Also applies to: 54-54
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastStoreFactoryTest.java (2)
35-35: LGTM! Factory correctly updated to useHazelcastPubSubEventStore.The import, builder usage, and factory construction are properly aligned with the renamed Hazelcast event store implementation.
Also applies to: 69-71
114-114: Assertion correctly updated.The type check properly validates against
HazelcastPubSubEventStore.class.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedKafkaSingleChannelTest.java (1)
52-52: Test updated correctly to use RedisStoreFactory.The import and constructor usages have been updated consistently for both test nodes. The constructor arguments match the new API signature.
Also applies to: 97-99, 144-146
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
37-37: Test updated correctly to use RedisStoreFactory with Redis Streams.The import and constructor usages have been updated consistently for both test nodes using
RedisStreamEventStoreinMULTI_CHANNELmode.Also applies to: 71-74, 117-120
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (1)
26-26: Test updated correctly to use HazelcastPubSubEventStore.The import and builder usage have been updated to use separate publish and subscribe Hazelcast instances, which aligns with the new API design.
Also applies to: 54-54
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (1)
38-38: Test updated correctly to use HazelcastPubSubRingBufferEventStore.The import on line 38 and builder usages on lines 78-79 and 123-124 have been updated consistently for both test nodes in
SINGLE_CHANNELmode. No remaining references to the oldHazelcastRingBufferEventStoreclass name exist in the codebase.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java (2)
37-38: API imports updated to match documentation.The import changes from
RedissonStoreFactory/RedissonEventStoretoRedisStoreFactory/RedisPubSubEventStorealign with the PR objective to rename APIs for version 4.0.0.
71-73: Store factory configuration correctly updated.Both node configurations properly use the new
RedisStoreFactoryconstructor withRedisPubSubEventStore.Builderto configure SINGLE_CHANNEL mode for distributed testing.Also applies to: 116-118
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubMultiChannelUnReliableTest.java (1)
37-38: API updates consistent with multi-channel test requirements.The changes mirror the single-channel test but correctly configure
EventStoreMode.MULTI_CHANNELfor both nodes, which is appropriate for this test class.Also applies to: 71-73, 116-118
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (1)
37-37: Factory renamed while preserving Redis Stream event store.The
RedisStoreFactoryrename is correctly applied while maintaining the use ofRedisStreamEventStore.Builder, demonstrating the factory's ability to work with different event store implementations.Also applies to: 71-71, 116-116
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java (1)
37-37: Hazelcast API updated consistently with Redis variants.The import and factory instantiation changes from
HazelcastEventStoretoHazelcastPubSubEventStorefollow the same naming pattern as the Redis variants, maintaining consistency across the codebase.Also applies to: 78-78, 123-123
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastPubSubRingBufferEventStore.java (3)
42-42: Class renamed to include PubSub qualifier.The rename from
HazelcastRingBufferEventStoretoHazelcastPubSubRingBufferEventStoreis consistent with the broader API renaming effort and appropriately identifies the pub/sub nature of this event store variant.
53-53: Logger reference correctly updated.The logger initialization properly references the renamed class, ensuring log statements will display the correct class name.
58-58: Constructor name updated to match class.The constructor name correctly matches the renamed class, maintaining proper Java syntax.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.java (3)
37-38: LGTM! Import updates align with the API renaming.The imports correctly reference the renamed classes (
RedisStoreFactoryandRedisPubSubReliableEventStore) matching the PR objective of aligning API names with documentation.
72-74: LGTM! Correctly configures Node1 with SINGLE_CHANNEL mode.The
RedisPubSubReliableEventStore.Builderusage is consistent with the API shown in the relevant code snippets—singleRedissonClientconstructor delegates to the pub/sub variant.
117-119: LGTM! Node2 configuration mirrors Node1 correctly.Both nodes use consistent
RedisStoreFactory+RedisPubSubReliableEventStorewiring withSINGLE_CHANNELmode.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubMultiChannelUnReliableTest.java (3)
37-38: LGTM! Import updated to the renamedHazelcastPubSubEventStoreclass.The import change aligns with the API renaming from
HazelcastEventStoretoHazelcastPubSubEventStore.
77-79: LGTM! Node1 correctly configured with HazelcastPubSubEventStore.The builder usage with
MULTI_CHANNELmode matches the test purpose and is consistent with the new API.
124-125: LGTM! Node2 mirrors Node1 configuration.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (3)
38-38: LGTM! Import updated to renamedHazelcastPubSubRingBufferEventStore.Aligns with the API renaming objective.
77-79: LGTM! Node1 correctly uses HazelcastPubSubRingBufferEventStore.Builder.The
MULTI_CHANNELmode is explicitly set, matching the test class name.
122-124: LGTM! Node2 configuration is consistent with Node1.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.java (3)
37-38: LGTM! Imports updated to renamed Redis classes.Consistent with the API renaming pattern across the codebase.
71-74: LGTM! Node1 correctly configured with MULTI_CHANNEL mode.The
RedisPubSubReliableEventStore.Builderusage withMULTI_CHANNELmatches the test class name.
117-120: LGTM! Node2 configuration mirrors Node1.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (6)
23-23: LGTM! Added@NotNullannotation import for null-safety.Good practice to use JetBrains annotations for compile-time null checking.
33-47: LGTM! Well-documented class javadoc.The documentation clearly explains the factory's purpose, hybrid configuration options, and the default behavior when no
EventStoreis supplied.
65-69: LGTM! Primary constructor with proper null-safety.The
@NotNullannotations combined withObjects.requireNonNullprovide both compile-time and runtime protection.
92-94: LGTM! Exposes the configuredEventStoreinstance.Required by the abstract
BaseStoreFactory.eventStore()method.
97-103: LGTM! Defensive shutdown with exception handling.Wrapping
eventStore.shutdown()in try-catch prevents shutdown failures from propagating and logs the error for diagnosability.
110-113: LGTM! UsefultoString()for debugging.Provides a descriptive representation of the factory instance.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (2)
144-157: LGTM! Core factory methods are well-implemented.
createStore(),eventStore(), andcreateMap()follow the same pattern asHazelcastStoreFactory.
159-171: LGTM! Shutdown and toString implementations are consistent.The try-catch in
shutdown()and descriptivetoString()match the pattern established inHazelcastStoreFactory.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastPubSubEventStore.java (4)
41-41: LGTM! Class renamed toHazelcastPubSubEventStore.The rename aligns with the PR objective of matching API names to documentation.
54-54: LGTM! Logger updated to reference the renamed class.
56-83: LGTM! Constructor implementation is correct.Proper null-safety with
Objects.requireNonNulland sensible defaults for optional parameters.
203-211: LGTM! Builder correctly returnsHazelcastPubSubEventStore.The
build()method properly instantiates the renamed class.
netty-socketio-core/src/main/java/com/socketio4j/socketio/Configuration.java
Show resolved
Hide resolved
.../com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastPubSubRingBufferEventStore.java
Show resolved
Hide resolved
...ocketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java
Show resolved
Hide resolved
...ocketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java
Show resolved
Hide resolved
...ocketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java
Show resolved
Hide resolved
...ocketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java
Show resolved
Hide resolved
...cketio-core/src/test/java/com/socketio4j/socketio/store/event/RedisPubSubEventStoreTest.java
Show resolved
Hide resolved
...ketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java
Outdated
Show resolved
Hide resolved
...ketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (1)
28-30: Test class name doesn't match the implementation being tested.The class name
HazelcastRingBufferEventStoreTestsuggests it tests a RingBuffer-based event store, but it actually testsHazelcastPubSubEventStore, which uses Topics (ITopic) for pub/sub messaging. The comment on line 28 mentions "HazelcastPubSubStore," which aligns with the implementation but not the class name.Consider renaming this test class to
HazelcastPubSubEventStoreTestto match the implementation being tested.🔎 Suggested fix
/** - * Test class for HazelcastPubSubStore using testcontainers + * Test class for HazelcastPubSubEventStore using testcontainers */ -public class HazelcastRingBufferEventStoreTest extends AbstractEventStoreTest { +public class HazelcastPubSubEventStoreTest extends AbstractEventStoreTest {
🧹 Nitpick comments (1)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java (1)
45-47: Consider renaming for consistency with the refactored API.The test class name, comments, and variable names still reference "Redisson" specifically, while the actual implementation under test is now
RedisStoreFactory. For clarity and consistency with the refactored API, consider:
- Renaming the class from
RedissonReliableStoreFactoryTesttoRedisStoreFactoryTest- Updating the class-level comment (line 45) to reference
RedisStoreFactory- Updating method comments to be more generic (e.g., "Test that the factory creates Redis-backed stores" on line 91)
- Using more generic variable names (e.g.,
redisKeyinstead ofredissonKey)This would better reflect that the public API is now Redis-focused rather than Redisson-specific, even though Redisson remains the underlying implementation.
Also applies to: 91-91, 98-100, 113-113
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedisEventStoreTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java
🧰 Additional context used
🧬 Code graph analysis (2)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastPubSubEventStore.java (1)
HazelcastPubSubEventStore(41-214)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisPubSubEventStore.java (1)
RedisPubSubEventStore(43-200)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStore.java (1)
RedisStore(27-60)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedisStoreFactory.java (1)
RedisStoreFactory(107-172)
⏰ 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 (14)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java (4)
22-24: LGTM!The import statements correctly reflect the renamed classes from the refactoring.
64-65: LGTM!The factory instantiation correctly uses the new
RedisStoreFactoryconstructor with theRedisPubSubEventStore.Builder, matching the refactored API.
96-96: LGTM!The assertion message now correctly matches the type being checked. The previous review comment has been addressed.
108-108: LGTM!The assertion message now correctly matches the type being checked. The previous review comment has been addressed.
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedisEventStoreTest.java (1)
30-30: Verify test intent and consider more specific class name.The class has been renamed from
RedissonReliableEventStoreTesttoRedisEventStoreTest, which removes the "Reliable" claim but is still generic. Since the test instantiatesRedisPubSubEventStore(line 49), consider renaming toRedisPubSubEventStoreTestfor clarity.Additionally, per the previous review,
RedisPubSubEventStoreis documented as the unreliable pub/sub implementation. Please verify:
- Is testing the unreliable store intentional?
- If a reliable store test is needed, should this use
RedisPubSubReliableEventStoreinstead?This partially addresses the past review concern but could benefit from clearer naming.
Also applies to: 49-49
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (3)
25-25: LGTM! Import updated correctly.The import has been correctly updated to reflect the renamed class
HazelcastPubSubEventStore.
41-54: LGTM! Builder usage updated correctly.The builder invocation has been correctly updated to use
HazelcastPubSubEventStore.Builder, and the usage pattern aligns with the new API shown in the implementation.
57-61: LGTM! Resource cleanup is appropriate.The teardown method properly cleans up both Hazelcast client instances and the container with appropriate null checks.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (6)
34-47: Excellent documentation of the factory's pluggable design.The class-level Javadoc clearly explains the separation between session storage (Hazelcast) and event propagation (pluggable EventStore), and provides helpful examples of hybrid configurations.
55-69: Strong null-safety enforcement on the primary constructor.The
@NotNullannotations combined with explicit null checks provide clear contracts and fail-fast behavior.
92-94: Clean public accessor for the EventStore.The method provides appropriate public access to the configured event store implementation.
97-103: Robust error handling in shutdown.The try-catch block with error logging ensures that shutdown failures don't propagate, improving the factory's lifecycle management reliability.
110-114: Helpful toString() implementation for debugging.The descriptive output will aid in logging and debugging scenarios.
71-84: All verification points are confirmed correct. TheHazelcastPubSubEventStoreconstructor signature matches the call inHazelcastStoreFactory(line 83), and passingnullfor botheventStoreModeandtopicPrefixcorrectly defaults toEventStoreMode.MULTI_CHANNELand"SOCKETIO4J:"respectively, as documented in the Javadoc.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedisPubSubEventStoreTest.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 (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (1)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedisPubSubEventStoreTest.java (1)
25-25: LGTM! Test class rename addresses previous feedback.The test class has been correctly renamed to
RedisPubSubEventStoreTest, and the import and instantiation are now consistent. This properly addresses the previous review comment about the name/implementation mismatch.Also applies to: 30-30, 49-49
| /** | ||
| * Test class for RedissonReliableEventStore using testcontainers | ||
| * Test class for RedisPubSubEventStoreTest using testcontainers | ||
| */ |
There was a problem hiding this comment.
Fix Javadoc typo: reference the implementation, not the test class.
The Javadoc should reference RedisPubSubEventStore (the implementation under test), not RedisPubSubEventStoreTest (the test class itself).
🔎 Proposed fix
/**
- * Test class for RedisPubSubEventStoreTest using testcontainers
+ * Test class for RedisPubSubEventStore using testcontainers
*/📝 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.
| /** | |
| * Test class for RedissonReliableEventStore using testcontainers | |
| * Test class for RedisPubSubEventStoreTest using testcontainers | |
| */ | |
| /** | |
| * Test class for RedisPubSubEventStore using testcontainers | |
| */ |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedisPubSubEventStoreTest.java
around lines 27 to 29, the Javadoc incorrectly references the test class name;
update the Javadoc comment to reference the implementation under test,
RedisPubSubEventStore, instead of RedisPubSubEventStoreTest. Edit the Javadoc
sentence to mention RedisPubSubEventStore (and keep the rest of the comment
intact), then save and run tests to ensure no further documentation linter
issues.
Description
Storefactory api final 4.0.0
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
Release Notes
New Features
eventStore()method to expose configured event store instances.Bug Fixes
Refactor
toString()methods.✏️ Tip: You can customize this high-level summary in your review settings.