Refactored + Added New Commands - #41
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds multiple Redis-like commands (PING, ECHO, LLEN, LPOP, BLPOP, EXPIRE, TTL), refactors request decoding to support pipelining/fragmentation, extends expiry/DB singleton APIs, centralizes Maven config, updates docs and gitignore, and adds extensive unit/integration tests plus test orchestration scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as RedisCommandHandler
participant DB as RedisDatabase
participant Expiry as ExpiryManager
Client->>Handler: Send BLPOP key1 key2 <timeout>
Handler->>DB: compute/pop attempt on key1
alt element available
DB-->>Handler: element + key
Handler-->>Client: RESP array [key, element]
else no element
Handler->>Handler: schedule/poll loop (event-loop or sync test fallback)
loop until timeout or element found
Handler->>DB: compute/pop attempt on keys...
DB-->>Handler: null or element
end
alt element found later
Handler-->>Client: RESP array [key, element]
else timeout
Handler-->>Client: RESP nil
end
end
note right of Expiry: ExpiryManager may asynchronously remove expired keys referenced by DB
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @unikdahal. * #41 (comment) The following files were modified: * `run_all_tests.sh` * `src/main/java/com/redis/commands/BLPopCommand.java` * `src/main/java/com/redis/commands/CommandRegistry.java` * `src/main/java/com/redis/commands/EchoCommand.java` * `src/main/java/com/redis/commands/ExpireCommand.java` * `src/main/java/com/redis/commands/LLenCommand.java` * `src/main/java/com/redis/commands/LPopCommand.java` * `src/main/java/com/redis/commands/PingCommand.java` * `src/main/java/com/redis/commands/TtlCommand.java` * `src/main/java/com/redis/server/RedisCommandHandler.java` * `src/main/java/com/redis/storage/ExpiryManager.java` * `src/main/java/com/redis/storage/RedisDatabase.java` * `src/main/test/integration_tests.sh`
📝 Add docstrings to `feature/LPushCommand`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 9-10: Add the missing ignore for the build/ directory to
.gitignore and remove the currently committed build artifacts from the repo
index so they stop being tracked; specifically, add the pattern "build/" to
.gitignore, run a cached removal of the tracked build files (e.g., remove
build/classes/ and related build outputs from the index), and commit both the
updated .gitignore and the removal so future builds are untracked. Ensure you
verify the repo no longer lists build/ files as changes after committing.
In `@build/classes/application.properties`:
- Around line 1-19: Remove the committed build artifact application.properties
from version control (the compiled file in your build output) by running git rm
--cached on that file and commit the removal, and update your .gitignore to
include the build/ directory (add a line "build/") so future build outputs are
ignored; ensure the canonical source application.properties in
src/main/resources remains tracked.
In `@build/classes/META-INF/INDEX.LIST`:
- Around line 1-8: Remove the tracked build output and the stale jar index
referencing netty-all-4.1.94.Final.jar by deleting the build/classes/ directory
(which contains META-INF/INDEX.LIST) from the repo and committing that deletion;
then update .gitignore to include build/classes/ (and any other build output
patterns) so generated artifacts aren’t re-added, and verify pom.xml's modular
Netty dependencies (not netty-all) remain as the source of truth.
In `@build/classes/META-INF/io.netty.versions.properties`:
- Around line 1-9: The checked-in Netty metadata file
io.netty.versions.properties under build/classes/ is a generated build artifact
and should be removed from the repo; delete io.netty.versions.properties and any
other files under build/classes/, add the build/ directory (or at minimum
build/classes/) to .gitignore so generated files are not tracked, and commit a
removal commit that updates the repository state accordingly.
In `@build/classes/META-INF/MANIFEST.MF`:
- Around line 1-27: The committed MANIFEST.MF contains stale Netty artifact
metadata (e.g., Implementation-Version: 4.1.94.Final, Build-Jdk: 1.8.0_252,
Bundle-Name: Netty/All-in-One) and should be removed: delete this MANIFEST.MF
and the entire build/ directory from version control, add an entry to ignore
build/ in .gitignore (or similar VCS ignore) to prevent re-committing, and
commit the removal with a brief message like “remove stale Netty build
artifacts.”
In `@build/classes/META-INF/maven/io.netty/netty-all/pom.properties`:
- Around line 1-4: Remove the committed Maven build artifact (pom.properties)
from source control and prevent future build outputs from being tracked: add a
rule to .gitignore to exclude the build/ directory, remove the tracked file from
the repository index (e.g., git rm --cached for the artifact), and commit the
change so the file is no longer present in the repo while remaining in local
build outputs.
In `@build/classes/META-INF/maven/io.netty/netty-all/pom.xml`:
- Around line 20-22: A checked-in build artifact contains a vulnerable Netty
dependency (pom.xml with groupId "io.netty", artifactId "netty-all" and version
"4.1.94.Final") that must be removed and the root build updated: delete the
committed build artifact file, then update the Netty dependency in the root
pom.xml to at least version "4.1.130.Final" (replace any occurrences of
"io.netty:netty-all:4.1.94.Final" or similar entries) so all listed CVEs are
patched.
In `@pom.xml`:
- Around line 170-179: The exec-maven-plugin configuration is passing
--enable-preview as a program argument (inside <arguments>) to
com.redis.server.NettyRedisServer instead of as a JVM option; move the flag into
the plugin's <configuration><jvmArgs> section (e.g., add
<jvmArgs><jvmArg>--enable-preview</jvmArg></jvmArgs>) so the JVM runs with
preview enabled while keeping <arguments> for program args and preserving the
existing <mainClass> value.
In `@README.md`:
- Around line 3-5: The README contains a Build Status badge whose link target is
a placeholder "#" — locate the markdown badge line for the Build Status (the
line with "[](#)") and either
replace the "#" with the real CI build URL (e.g., your GitHub Actions or other
CI badge link) or remove the entire badge line until a valid CI URL is
available; ensure the README ends up with no placeholder links.
In `@run_all_tests.sh`:
- Around line 7-9: The script currently hard-codes
PROJECT_DIR="/home/unik/Coding/backend/redis-java" and then cds to
"$PROJECT_DIR", which breaks portability; change PROJECT_DIR to be derived from
the script location or overrideable via an environment variable (e.g., set
PROJECT_DIR=${PROJECT_DIR:-<derived path using script dir>} ), then keep the
existing cd "$PROJECT_DIR" so callers can either export PROJECT_DIR or rely on
the script-located path; update references to PROJECT_DIR in the script
accordingly.
In `@src/main/java/com/redis/commands/BLPopCommand.java`:
- Around line 87-92: The BLPopCommand currently treats a timeout of 0 as an
immediate no-result (see the timeout handling branch in BLPopCommand), which
deviates from Redis semantics where timeout 0 blocks indefinitely; either
implement true indefinite blocking by replacing the immediate-break behavior
with an asynchronous wait (e.g., register a listener/CompletableFuture or use
the server's list-notify mechanism to resume the command when an element is
pushed) in the BLPopCommand execution flow, or explicitly document the
limitation by updating BLPopCommand's javadoc and the public compatibility
notes/README to state that timeout==0 returns nil instead of blocking
indefinitely; locate the timeout check in BLPopCommand and either (A) wire into
the server's async notification/wait facility and complete the blocking future
on push, or (B) add a clear comment and public documentation change describing
the non-standard behavior.
- Around line 64-93: The BLPopCommand currently uses Thread.sleep() inside its
blocking loop which will block Netty's event loop; replace the blocking loop in
BLPopCommand (the code using Thread.sleep and POLL_INTERVAL_MS) with an
asynchronous scheduled retry using the ChannelHandlerContext executor: extract
the check logic that iterates keys and calls tryPopFromKey and formatResult into
a helper (e.g., scheduleCheck) and use ctx.executor().schedule(...,
POLL_INTERVAL_MS, TimeUnit.MILLISECONDS) to re-schedule the checks until
deadline or a result is found, writing RESP_NIL or the formatted result via
ctx.writeAndFlush; remove any Thread.sleep/InterruptedException handling so the
event loop is never blocked.
- Around line 131-135: The BLPopCommand currently treats wrongType.get() as a
signal to skip keys and return null; instead, when any key is a non-list you
must immediately return a WRONGTYPE error to the client. Update the logic in
BLPopCommand where wrongType.get() is checked so that it does not return null or
continue scanning, but triggers the existing error response path (e.g., throw or
call the server's WRONGTYPE error helper / sendWrongTypeError) as soon as
wrongType.get() is true, ensuring BLPOP replies immediately with a WRONGTYPE
error for the first non-list key encountered.
In `@src/main/java/com/redis/commands/EchoCommand.java`:
- Around line 10-14: The RESP bulk string length is computed using Java char
count which breaks for multibyte UTF-8; in EchoCommand replace the use of
msg.length() with the actual byte length (e.g.
msg.getBytes(StandardCharsets.UTF_8).length) when building the
"$<len>\r\n<msg>\r\n" response, and add the necessary import for
StandardCharsets if missing.
In `@src/main/java/com/redis/commands/PingCommand.java`:
- Around line 11-18: The execute method in PingCommand allows more than one
argument and incorrectly returns RESP_PONG; add explicit argument count
validation in execute: if args != null and args.size() > 1 return the Redis
error string for wrong argument count (e.g. "-ERR wrong number of arguments for
'ping' command\r\n"); keep the existing branch for exactly one argument (echo as
bulk string) and the fallback return of RESP_PONG for zero arguments. Ensure you
reference the execute method and RESP_PONG constant when making this change.
In `@src/main/java/com/redis/commands/TtlCommand.java`:
- Around line 18-20: The execute method in TtlCommand currently only checks for
empty args and ignores extra arguments; update the arity validation in
com.redis.commands.TtlCommand.execute to require exactly one argument
(args.size() != 1) and return ERR_WRONG_ARGS when the count is not one, so extra
arguments are rejected per Redis spec; keep the rest of the method logic and
ChannelHandlerContext usage unchanged.
In `@src/main/java/com/redis/server/RedisCommandHandler.java`:
- Around line 63-105: parseRespArray currently swallows protocol errors by
returning false, which makes the decoder re-read the same bad byte and stall;
change it so only incomplete data (readInteger == INCOMPLETE or
buf.readableBytes checks) returns false, but any malformed frame conditions
(e.g., missing '*' marker check after buf.readByte(), unexpected marker != '$',
negative lengths that are not the defined RESP nil semantics, or inconsistent
readableBytes < strLen+2) throw a protocol exception (e.g.,
CorruptedFrameException or a custom ProtocolException) instead of returning
false; also remove or narrow the broad try/catch so exceptions propagate to
exceptionCaught for proper channel close. Ensure references: parseRespArray,
readInteger, INCOMPLETE, and exceptionCaught when making the change.
In `@src/test/java/com/redis/commands/ExpireTtlCommandTest.java`:
- Around line 38-47: Replace the fixed Thread.sleep(3000) in
ExpireTtlCommandTest with a polling loop that repeatedly calls
ttl.execute(List.of(key), ctx) until it returns ":-2\r\n" or a short timeout
elapses (e.g., 5 seconds); implement the loop using System.currentTimeMillis()
to check elapsed time and Thread.sleep with a small interval (e.g., 50–100ms)
between polls, then assert that the final ttl.execute result equals ":-2\r\n" to
fail the test if the timeout is reached.
In `@src/test/java/com/redis/commands/LLenCommandTest.java`:
- Around line 77-96: The test testConcurrentSafePreserveTTLWithLPush uses fixed
Thread.sleep calls causing flakiness; replace them with bounded polling loops
that repeatedly check db.getValue("ttl-list") with short sleeps (e.g., 50–100ms)
and a hard timeout instead of the fixed Thread.sleep durations. Specifically,
after calling LPushCommand.execute(...) assert the key still exists by looping
until either db.getValue("ttl-list") != null or a 1.5s timeout elapses; then for
the post-expiry check loop until db.getValue("ttl-list") == null or a 3s timeout
elapses. Keep references to db.put(..., 2000), LPushCommand, and
db.getValue("ttl-list") so the replacement logic is applied in the same test
method.
In `@src/test/java/com/redis/server/PipeliningTest.java`:
- Around line 25-29: The test reads outbound ByteBufs (response1 and response2)
from channel.readOutbound() but never releases them; update the test to release
each ByteBuf after asserting (use response1.release()/response2.release() or
ReferenceCountUtil.release(...)) and/or call channel.finishAndReleaseAll() or
channel.close() in the test teardown to ensure EmbeddedChannel cleans up
resources; locate the usages of response1, response2, and channel.readOutbound()
in PipeliningTest and add the releases/final cleanup accordingly.
🧹 Nitpick comments (11)
build/MANIFEST.MF (1)
1-3: Build artifact should not be tracked in version control.The
build/directory appears to be a build output directory. ThisMANIFEST.MFfile will be auto-generated by the Maven Shade plugin (configured inpom.xmlwithManifestResourceTransformer). Tracking build artifacts in version control can cause merge conflicts and inconsistencies.Consider adding
build/to your.gitignoreand removing these files from the repository.README.md (1)
22-47: Add blank lines around tables for proper Markdown rendering.Tables should be surrounded by blank lines per Markdown best practices. This ensures consistent rendering across different Markdown parsers.
Suggested fix (example for first table)
### 🔑 Connection & Utility + | Command | Usage | Description | |:---|:---|:---| | `PING` | `PING [message]` | Returns `PONG` or the provided message. | | `ECHO` | `ECHO message` | Returns the provided message. | | `EXPIRE` | `EXPIRE key seconds` | Set a timeout on a key. | | `TTL` | `TTL key` | Get the time to live for a key in seconds. | + ### 📝 Key-Value OperationsApply similar changes around lines 33 and 40.
src/test/java/com/redis/storage/ExpiryBugTest.java (1)
6-55: Test isolation concern with singleton usage.Both tests use
RedisDatabase.getInstance()which returns a shared singleton. This can cause test pollution if tests run in parallel or if state leaks between tests. Consider adding a@BeforeEachcleanup method to delete the test keys, or use unique key prefixes per test run.Also, consider adding
@Timeoutannotations to prevent tests from hanging indefinitely if the sleep-based assertions fail.♻️ Suggested improvement for test isolation
public class ExpiryBugTest { + `@org.junit.jupiter.api.BeforeEach` + public void cleanup() { + RedisDatabase db = RedisDatabase.getInstance(); + db.remove("bugKey"); + db.remove("bugKeyDelete"); + } + `@Test` + `@org.junit.jupiter.api.Timeout`(5) public void testReAddWithoutTTLAfterTTL() throws InterruptedException {src/main/java/com/redis/commands/ExpireCommand.java (2)
8-13: Docstring mentions unimplemented options.The docstring references
[NX | XX | GT | LT]options that aren't implemented. Either remove these from the docstring or add a note that they're not supported in this implementation to avoid confusion.📝 Suggested docstring fix
/** - * EXPIRE key seconds [NX | XX | GT | LT] + * EXPIRE key seconds * Set a timeout on key. After the timeout has expired, the key will automatically be deleted. - * - * Simple implementation supporting seconds. + * + * Simple implementation supporting seconds only. NX/XX/GT/LT options are not implemented. */
19-28: Missing upper bound validation for argument count.The validation
args.size() < 2catches too few arguments, but extra arguments are silently ignored. Consider strict validation to match Redis behavior, which returns an error for wrong argument count.♻️ Suggested fix
`@Override` public String execute(List<String> args, ChannelHandlerContext ctx) { - if (args.size() < 2) return ERR_WRONG_ARGS; + if (args.size() != 2) return ERR_WRONG_ARGS; String key = args.get(0);src/main/test/integration_tests.sh (2)
19-19: Unquoted array assignment can cause issues with special characters.The word splitting on
$cmdcan produce unexpected results if arguments contain spaces or glob characters. Useread -afor safer parsing.♻️ Suggested fix
- local args=($cmd) + local args + read -ra args <<< "$cmd"
1-5: Non-standard test directory location.The file is located at
src/main/test/but Maven convention places tests undersrc/test/. This may cause the script to be excluded from standard test discovery or included in production artifacts.src/main/java/com/redis/commands/LPopCommand.java (1)
71-72: Minor inconsistency: Consider usingasList()for type-safe access.After verifying the type is
LIST, you cast viagetData()directly. While this works, usingexisting.asList()would be more consistent with howLLenCommandaccesses the list and provides built-in type safety.♻️ Suggested change
- `@SuppressWarnings`("unchecked") - List<String> existingList = (List<String>) existing.getData(); + List<String> existingList = existing.asList();src/test/java/com/redis/commands/LPopCommandTest.java (1)
6-29: Prefer MockitoExtension to manage mock lifecycle.
MockitoAnnotations.openMocksshould be closed; using the JUnit 5 extension avoids leaks and boilerplate.♻️ Proposed refactor
+import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; +import org.mockito.junit.jupiter.MockitoExtension; +@ExtendWith(MockitoExtension.class) class LPopCommandTest { @@ `@BeforeEach` void setUp() { - MockitoAnnotations.openMocks(this); command = new LPopCommand(); db = RedisDatabase.getInstance(); }src/test/java/com/redis/commands/BLPopCommandTest.java (2)
69-79: Time-based assertions can be flaky under CI jitter.
Consider widening bounds to reduce intermittent failures.🧪 Suggested tweak
- assertTrue(elapsed >= 80 && elapsed < 500, "Should wait approximately 100ms"); + assertTrue(elapsed >= 50 && elapsed < 1500, "Should wait approximately 100ms"); @@ - assertTrue(elapsed >= 150 && elapsed < 500); + assertTrue(elapsed >= 150 && elapsed < 1500);Also applies to: 186-194
196-218: Ensure executor is always shut down (even on assertion failure).
Use atry/finallyto avoid thread leaks in failing tests.♻️ Suggested refactor
- ExecutorService executor = Executors.newSingleThreadExecutor(); - Future<String> future = executor.submit(() -> - command.execute(List.of("blocktest", "2"), ctx) - ); - - // Wait a bit, then push data - Thread.sleep(100); - LPushCommand lpush = new LPushCommand(); - lpush.execute(List.of("blocktest", "arrived"), ctx); - - // BLPOP should return with the data - String result = future.get(2, TimeUnit.SECONDS); - assertTrue(result.contains("blocktest")); - assertTrue(result.contains("arrived")); - - executor.shutdown(); - db.remove("blocktest"); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future<String> future = executor.submit(() -> + command.execute(List.of("blocktest", "2"), ctx) + ); + + // Wait a bit, then push data + Thread.sleep(100); + LPushCommand lpush = new LPushCommand(); + lpush.execute(List.of("blocktest", "arrived"), ctx); + + // BLPOP should return with the data + String result = future.get(2, TimeUnit.SECONDS); + assertTrue(result.contains("blocktest")); + assertTrue(result.contains("arrived")); + } finally { + executor.shutdownNow(); + db.remove("blocktest"); + }
| # Maven | ||
| target/ |
There was a problem hiding this comment.
Add build/ directory to .gitignore.
The build/ directory containing compiled classes and Maven metadata is missing from the ignore patterns. This PR includes committed build artifacts under build/classes/ that should be excluded.
Proposed fix
# Maven
target/
+build/
pom.xml.tag📝 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.
| # Maven | |
| target/ | |
| # Maven | |
| target/ | |
| build/ |
🤖 Prompt for AI Agents
In @.gitignore around lines 9 - 10, Add the missing ignore for the build/
directory to .gitignore and remove the currently committed build artifacts from
the repo index so they stop being tracked; specifically, add the pattern
"build/" to .gitignore, run a cached removal of the tracked build files (e.g.,
remove build/classes/ and related build outputs from the index), and commit both
the updated .gitignore and the removal so future builds are untracked. Ensure
you verify the repo no longer lists build/ files as changes after committing.
| # Redis Java Server Configuration | ||
| # Default values are provided in RedisConfig class | ||
|
|
||
| # Server port | ||
| redis.port=6379 | ||
|
|
||
| # Netty Boss Group thread count (accepts connections) | ||
| redis.boss.threads=1 | ||
|
|
||
| # Netty Worker Group thread count (handles I/O) | ||
| # Single-threaded model: 1 thread ensures sequential command processing like real Redis | ||
| redis.worker.threads=1 | ||
|
|
||
| # Cleanup interval in milliseconds (used for periodic safety checks) | ||
| # Note: Main expiry is DelayQueue-based (zero-polling), this is just a backup | ||
| redis.cleanup.interval.ms=5000 | ||
|
|
||
| # Enable expiry management for TTL keys | ||
| redis.expiry.enabled=true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for the source application.properties file and .gitignore configuration
# Check if source file exists
echo "=== Checking for source file ==="
fd -t f "application.properties" src/main/resources/ 2>&1
# Check .gitignore for build directory exclusions
echo -e "\n=== Checking .gitignore for build directory exclusions ==="
if [ -f .gitignore ]; then
echo "Searching for 'build/' in .gitignore:"
rg -n "build/" .gitignore || echo "Warning: 'build/' not found in .gitignore"
echo -e "\nSearching for 'target/' in .gitignore:"
rg -n "target/" .gitignore || echo "Warning: 'target/' not found in .gitignore"
else
echo "Warning: .gitignore file not found"
fi
# List all application.properties files in the repository
echo -e "\n=== All application.properties files in repository ==="
fd -t f "application.properties"
# Check if build directory exists
echo -e "\n=== Checking build directory ==="
ls -la build/ 2>&1 | head -20Repository: unikdahal/redis-java
Length of output: 723
Remove build artifact from version control and update .gitignore.
The file build/classes/application.properties should not be committed. This is a build output directory containing compiled artifacts. The correct source file already exists at src/main/resources/application.properties.
Actions required:
- Remove
build/classes/application.propertiesfrom version control - Add
build/to.gitignore(currently onlytarget/is excluded)
🤖 Prompt for AI Agents
In `@build/classes/application.properties` around lines 1 - 19, Remove the
committed build artifact application.properties from version control (the
compiled file in your build output) by running git rm --cached on that file and
commit the removal, and update your .gitignore to include the build/ directory
(add a line "build/") so future build outputs are ignored; ensure the canonical
source application.properties in src/main/resources remains tracked.
| JarIndex-Version: 1.0 | ||
|
|
||
| netty-all-4.1.94.Final.jar | ||
| META-INF | ||
| META-INF/maven | ||
| META-INF/maven/io.netty | ||
| META-INF/maven/io.netty/netty-all | ||
|
|
There was a problem hiding this comment.
Build artifact with version mismatch should be removed.
This file references netty-all-4.1.94.Final.jar but the pom.xml specifies version 4.1.118.Final. Additionally, the pom.xml now uses modular Netty dependencies instead of netty-all.
This build/classes/ directory contains build output that should not be tracked in version control. Remove this directory and add it to .gitignore.
🤖 Prompt for AI Agents
In `@build/classes/META-INF/INDEX.LIST` around lines 1 - 8, Remove the tracked
build output and the stale jar index referencing netty-all-4.1.94.Final.jar by
deleting the build/classes/ directory (which contains META-INF/INDEX.LIST) from
the repo and committing that deletion; then update .gitignore to include
build/classes/ (and any other build output patterns) so generated artifacts
aren’t re-added, and verify pom.xml's modular Netty dependencies (not netty-all)
remain as the source of truth.
| #Generated by netty-parent/pom.xml | ||
| #Tue, 20 Jun 2023 15:35:15 +0200 | ||
|
|
||
| netty-all.version=4.1.94.Final | ||
| netty-all.buildDate=2023-06-20 15\:35\:15 +0200 | ||
| netty-all.commitDate=2023-06-20 12\:54\:19 +0000 | ||
| netty-all.shortCommitHash=da1a448d5b | ||
| netty-all.longCommitHash=da1a448d5bc4f36cc1744db93fcaf64e198db2bd | ||
| netty-all.repoStatus=clean |
There was a problem hiding this comment.
Build artifact should not be committed.
This Netty-generated metadata file is a build artifact. Remove it along with the other files under build/classes/ and ensure the build/ directory is added to .gitignore.
🤖 Prompt for AI Agents
In `@build/classes/META-INF/io.netty.versions.properties` around lines 1 - 9, The
checked-in Netty metadata file io.netty.versions.properties under build/classes/
is a generated build artifact and should be removed from the repo; delete
io.netty.versions.properties and any other files under build/classes/, add the
build/ directory (or at minimum build/classes/) to .gitignore so generated files
are not tracked, and commit a removal commit that updates the repository state
accordingly.
| Manifest-Version: 1.0 | ||
| Implementation-Title: Netty/All-in-One | ||
| Bundle-Description: Netty is an asynchronous event-driven network appl | ||
| ication framework for rapid development of maintainable high perfo | ||
| rmance protocol servers and clients. | ||
| Automatic-Module-Name: io.netty.all | ||
| Bundle-License: https://www.apache.org/licenses/LICENSE-2.0 | ||
| Bundle-SymbolicName: io.netty.all | ||
| Implementation-Version: 4.1.94.Final | ||
| Built-By: norman | ||
| Bnd-LastModified: 1687268117627 | ||
| Bundle-ManifestVersion: 2 | ||
| Implementation-Vendor-Id: io.netty | ||
| Bundle-DocURL: https://netty.io/ | ||
| Bundle-Vendor: The Netty Project | ||
| Import-Package: sun.nio.ch;resolution:=optional,org.eclipse.jetty.npn; | ||
| version="[1,2)";resolution:=optional,org.eclipse.jetty.alpn;version=" | ||
| [1,2)";resolution:=optional | ||
| Tool: Bnd-6.3.1.202206071316 | ||
| Implementation-Vendor: The Netty Project | ||
| Bundle-Name: Netty/All-in-One | ||
| Bundle-Version: 4.1.94.Final | ||
| Build-Jdk-Spec: 1.8 | ||
| Created-By: Apache Maven Bundle Plugin 5.1.8 | ||
| Build-Jdk: 1.8.0_252 | ||
| Implementation-URL: https://netty.io/netty-all/ | ||
|
|
There was a problem hiding this comment.
Stale Netty build artifact should be removed.
This manifest is from Netty 4.1.94.Final (built with JDK 1.8), but the project now uses Netty 4.1.118.Final with Java 25. This appears to be an extracted dependency artifact that was accidentally committed.
Remove this file along with the entire build/ directory from version control.
🤖 Prompt for AI Agents
In `@build/classes/META-INF/MANIFEST.MF` around lines 1 - 27, The committed
MANIFEST.MF contains stale Netty artifact metadata (e.g.,
Implementation-Version: 4.1.94.Final, Build-Jdk: 1.8.0_252, Bundle-Name:
Netty/All-in-One) and should be removed: delete this MANIFEST.MF and the entire
build/ directory from version control, add an entry to ignore build/ in
.gitignore (or similar VCS ignore) to prevent re-committing, and commit the
removal with a brief message like “remove stale Netty build artifacts.”
| public String execute(List<String> args, ChannelHandlerContext ctx) { | ||
| if (args.isEmpty()) return ERR_WRONG_ARGS; | ||
|
|
There was a problem hiding this comment.
Fix TTL arity validation (extra args currently ignored).
Redis expects exactly one argument; accepting more silently deviates from spec and can mask client errors.
🐛 Proposed fix
- if (args.isEmpty()) return ERR_WRONG_ARGS;
+ if (args.size() != 1) return ERR_WRONG_ARGS;📝 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.
| public String execute(List<String> args, ChannelHandlerContext ctx) { | |
| if (args.isEmpty()) return ERR_WRONG_ARGS; | |
| public String execute(List<String> args, ChannelHandlerContext ctx) { | |
| if (args.size() != 1) return ERR_WRONG_ARGS; | |
🤖 Prompt for AI Agents
In `@src/main/java/com/redis/commands/TtlCommand.java` around lines 18 - 20, The
execute method in TtlCommand currently only checks for empty args and ignores
extra arguments; update the arity validation in
com.redis.commands.TtlCommand.execute to require exactly one argument
(args.size() != 1) and return ERR_WRONG_ARGS when the count is not one, so extra
arguments are rejected per Redis spec; keep the rest of the method logic and
ChannelHandlerContext usage unchanged.
| private boolean parseRespArray(ByteBuf buf, List<String> result) { | ||
| try { | ||
| if (buf.readableBytes() < 1) return false; | ||
|
|
||
| // Check for Array Start (*) | ||
| if (buf.readByte() != '*') { | ||
| return result; | ||
| return false; | ||
| } | ||
|
|
||
| // Read Array Length efficiently (no object allocation) | ||
| // Read Array Length | ||
| int numArgs = readInteger(buf); | ||
| if (numArgs <= 0) { | ||
| return result; | ||
| } | ||
|
|
||
| if (numArgs == INCOMPLETE) return false; | ||
| if (numArgs < 0) return true; // Nil array (*) or empty | ||
|
|
||
| // Read All Arguments | ||
| for (int i = 0; i < numArgs; i++) { | ||
| if (buf.readableBytes() < 1) return false; | ||
|
|
||
| // Check for Bulk String Start ($) | ||
| byte marker = buf.readByte(); | ||
| if (marker != '$') { | ||
| break; // Malformed | ||
| } | ||
| if (marker != '$') return false; | ||
|
|
||
| // Read String Length efficiently | ||
| // Read String Length | ||
| int strLen = readInteger(buf); | ||
| if (strLen == INCOMPLETE) return false; | ||
| if (strLen < 0) { | ||
| break; // Nil bulk string | ||
| result.add(null); | ||
| continue; | ||
| } | ||
|
|
||
| // Read the actual String data with charset conversion | ||
| if (buf.readableBytes() < strLen + 2) return false; | ||
|
|
||
| // Read the actual String data | ||
| CharSequence arg = buf.readCharSequence(strLen, StandardCharsets.UTF_8); | ||
| result.add(arg.toString()); | ||
|
|
||
| // Skip trailing \r\n | ||
| buf.skipBytes(2); | ||
| } | ||
| return true; | ||
| } catch (Exception e) { | ||
| // Log parse error but don't crash | ||
| result.clear(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Malformed RESP frames currently stall the connection indefinitely.
parseRespArray returns false for malformed input, causing the decoder to reset and wait forever on the same bad byte. Consider throwing on malformed frames so the channel closes via exceptionCaught.
🐛 Suggested fix to surface protocol errors
- private boolean parseRespArray(ByteBuf buf, List<String> result) {
- try {
- if (buf.readableBytes() < 1) return false;
-
- // Check for Array Start (*)
- if (buf.readByte() != '*') {
- return false;
- }
+ private boolean parseRespArray(ByteBuf buf, List<String> result) {
+ if (buf.readableBytes() < 1) return false;
+
+ // Check for Array Start (*)
+ byte first = buf.readByte();
+ if (first != '*') {
+ throw new IllegalArgumentException("Malformed RESP: expected array");
+ }
@@
- byte marker = buf.readByte();
- if (marker != '$') return false;
+ byte marker = buf.readByte();
+ if (marker != '$') {
+ throw new IllegalArgumentException("Malformed RESP: expected bulk string");
+ }
@@
- return true;
- } catch (Exception e) {
- return false;
- }
+ return true;
}📝 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.
| private boolean parseRespArray(ByteBuf buf, List<String> result) { | |
| try { | |
| if (buf.readableBytes() < 1) return false; | |
| // Check for Array Start (*) | |
| if (buf.readByte() != '*') { | |
| return result; | |
| return false; | |
| } | |
| // Read Array Length efficiently (no object allocation) | |
| // Read Array Length | |
| int numArgs = readInteger(buf); | |
| if (numArgs <= 0) { | |
| return result; | |
| } | |
| if (numArgs == INCOMPLETE) return false; | |
| if (numArgs < 0) return true; // Nil array (*) or empty | |
| // Read All Arguments | |
| for (int i = 0; i < numArgs; i++) { | |
| if (buf.readableBytes() < 1) return false; | |
| // Check for Bulk String Start ($) | |
| byte marker = buf.readByte(); | |
| if (marker != '$') { | |
| break; // Malformed | |
| } | |
| if (marker != '$') return false; | |
| // Read String Length efficiently | |
| // Read String Length | |
| int strLen = readInteger(buf); | |
| if (strLen == INCOMPLETE) return false; | |
| if (strLen < 0) { | |
| break; // Nil bulk string | |
| result.add(null); | |
| continue; | |
| } | |
| // Read the actual String data with charset conversion | |
| if (buf.readableBytes() < strLen + 2) return false; | |
| // Read the actual String data | |
| CharSequence arg = buf.readCharSequence(strLen, StandardCharsets.UTF_8); | |
| result.add(arg.toString()); | |
| // Skip trailing \r\n | |
| buf.skipBytes(2); | |
| } | |
| return true; | |
| } catch (Exception e) { | |
| // Log parse error but don't crash | |
| result.clear(); | |
| return false; | |
| } | |
| private boolean parseRespArray(ByteBuf buf, List<String> result) { | |
| if (buf.readableBytes() < 1) return false; | |
| // Check for Array Start (*) | |
| byte first = buf.readByte(); | |
| if (first != '*') { | |
| throw new IllegalArgumentException("Malformed RESP: expected array"); | |
| } | |
| // Read Array Length | |
| int numArgs = readInteger(buf); | |
| if (numArgs == INCOMPLETE) return false; | |
| if (numArgs < 0) return true; // Nil array (*) or empty | |
| // Read All Arguments | |
| for (int i = 0; i < numArgs; i++) { | |
| if (buf.readableBytes() < 1) return false; | |
| // Check for Bulk String Start ($) | |
| byte marker = buf.readByte(); | |
| if (marker != '$') { | |
| throw new IllegalArgumentException("Malformed RESP: expected bulk string"); | |
| } | |
| // Read String Length | |
| int strLen = readInteger(buf); | |
| if (strLen == INCOMPLETE) return false; | |
| if (strLen < 0) { | |
| result.add(null); | |
| continue; | |
| } | |
| if (buf.readableBytes() < strLen + 2) return false; | |
| // Read the actual String data | |
| CharSequence arg = buf.readCharSequence(strLen, StandardCharsets.UTF_8); | |
| result.add(arg.toString()); | |
| // Skip trailing \r\n | |
| buf.skipBytes(2); | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/redis/server/RedisCommandHandler.java` around lines 63 -
105, parseRespArray currently swallows protocol errors by returning false, which
makes the decoder re-read the same bad byte and stall; change it so only
incomplete data (readInteger == INCOMPLETE or buf.readableBytes checks) returns
false, but any malformed frame conditions (e.g., missing '*' marker check after
buf.readByte(), unexpected marker != '$', negative lengths that are not the
defined RESP nil semantics, or inconsistent readableBytes < strLen+2) throw a
protocol exception (e.g., CorruptedFrameException or a custom ProtocolException)
instead of returning false; also remove or narrow the broad try/catch so
exceptions propagate to exceptionCaught for proper channel close. Ensure
references: parseRespArray, readInteger, INCOMPLETE, and exceptionCaught when
making the change.
| // TTL should be around 2 | ||
| String ttlResult = ttl.execute(List.of(key), ctx); | ||
| assertTrue(ttlResult.equals(":1\r\n") || ttlResult.equals(":2\r\n")); | ||
|
|
||
| // Wait 3 seconds | ||
| Thread.sleep(3000); | ||
|
|
||
| // TTL should be -2 (expired) | ||
| assertEquals(":-2\r\n", ttl.execute(List.of(key), ctx)); | ||
|
|
There was a problem hiding this comment.
Avoid fixed sleeps for expiry timing.
The fixed 3s sleep can be flaky if the expiry worker lags. Prefer polling until TTL reports -2 with a timeout.
✅ Suggested polling approach
- // Wait 3 seconds
- Thread.sleep(3000);
-
- // TTL should be -2 (expired)
- assertEquals(":-2\r\n", ttl.execute(List.of(key), ctx));
+ // Wait until TTL reports expired (max 5s)
+ long deadline = System.currentTimeMillis() + 5000;
+ String ttlAfter;
+ do {
+ Thread.sleep(100);
+ ttlAfter = ttl.execute(List.of(key), ctx);
+ } while (!":-2\r\n".equals(ttlAfter) && System.currentTimeMillis() < deadline);
+ assertEquals(":-2\r\n", ttlAfter);🤖 Prompt for AI Agents
In `@src/test/java/com/redis/commands/ExpireTtlCommandTest.java` around lines 38 -
47, Replace the fixed Thread.sleep(3000) in ExpireTtlCommandTest with a polling
loop that repeatedly calls ttl.execute(List.of(key), ctx) until it returns
":-2\r\n" or a short timeout elapses (e.g., 5 seconds); implement the loop using
System.currentTimeMillis() to check elapsed time and Thread.sleep with a small
interval (e.g., 50–100ms) between polls, then assert that the final ttl.execute
result equals ":-2\r\n" to fail the test if the timeout is reached.
| @Test | ||
| void testConcurrentSafePreserveTTLWithLPush() throws InterruptedException { | ||
| // Set a list with TTL and perform LPUSH (uses compute) and ensure TTL preserved | ||
| db.remove("ttl-list"); | ||
| LinkedList<String> list = new LinkedList<>(List.of("x")); | ||
| db.put("ttl-list", RedisValue.list(list), 2000); // 2 seconds | ||
|
|
||
| // perform LPUSH via command which uses compute | ||
| LPushCommand lpush = new LPushCommand(); | ||
| String result = lpush.execute(List.of("ttl-list", "y"), ctx); | ||
| assertTrue(result.startsWith(":")); | ||
|
|
||
| // Immediately check value still has TTL (can't access expiry directly, but expiry manager will remove after TTL) | ||
| // Sleep slightly less than TTL and ensure key still exists | ||
| Thread.sleep(1000); | ||
| assertNotNull(db.getValue("ttl-list")); | ||
|
|
||
| // Sleep until after TTL | ||
| Thread.sleep(1500); | ||
| assertNull(db.getValue("ttl-list")); |
There was a problem hiding this comment.
Reduce test flakiness from fixed sleeps.
This test can intermittently fail on slow CI due to timing jitter around the 2s TTL. Consider polling with a timeout or increasing TTL and using a bounded wait loop.
✅ More reliable wait loop
- db.put("ttl-list", RedisValue.list(list), 2000); // 2 seconds
+ db.put("ttl-list", RedisValue.list(list), 5000); // 5 seconds
// perform LPUSH via command which uses compute
LPushCommand lpush = new LPushCommand();
String result = lpush.execute(List.of("ttl-list", "y"), ctx);
assertTrue(result.startsWith(":"));
// Immediately check value still has TTL (can't access expiry directly, but expiry manager will remove after TTL)
// Sleep slightly less than TTL and ensure key still exists
- Thread.sleep(1000);
+ Thread.sleep(1000);
assertNotNull(db.getValue("ttl-list"));
- // Sleep until after TTL
- Thread.sleep(1500);
- assertNull(db.getValue("ttl-list"));
+ // Wait until after TTL (poll with timeout)
+ long deadline = System.currentTimeMillis() + 6000;
+ while (db.getValue("ttl-list") != null && System.currentTimeMillis() < deadline) {
+ Thread.sleep(100);
+ }
+ assertNull(db.getValue("ttl-list"));📝 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 | |
| void testConcurrentSafePreserveTTLWithLPush() throws InterruptedException { | |
| // Set a list with TTL and perform LPUSH (uses compute) and ensure TTL preserved | |
| db.remove("ttl-list"); | |
| LinkedList<String> list = new LinkedList<>(List.of("x")); | |
| db.put("ttl-list", RedisValue.list(list), 2000); // 2 seconds | |
| // perform LPUSH via command which uses compute | |
| LPushCommand lpush = new LPushCommand(); | |
| String result = lpush.execute(List.of("ttl-list", "y"), ctx); | |
| assertTrue(result.startsWith(":")); | |
| // Immediately check value still has TTL (can't access expiry directly, but expiry manager will remove after TTL) | |
| // Sleep slightly less than TTL and ensure key still exists | |
| Thread.sleep(1000); | |
| assertNotNull(db.getValue("ttl-list")); | |
| // Sleep until after TTL | |
| Thread.sleep(1500); | |
| assertNull(db.getValue("ttl-list")); | |
| `@Test` | |
| void testConcurrentSafePreserveTTLWithLPush() throws InterruptedException { | |
| // Set a list with TTL and perform LPUSH (uses compute) and ensure TTL preserved | |
| db.remove("ttl-list"); | |
| LinkedList<String> list = new LinkedList<>(List.of("x")); | |
| db.put("ttl-list", RedisValue.list(list), 5000); // 5 seconds | |
| // perform LPUSH via command which uses compute | |
| LPushCommand lpush = new LPushCommand(); | |
| String result = lpush.execute(List.of("ttl-list", "y"), ctx); | |
| assertTrue(result.startsWith(":")); | |
| // Immediately check value still has TTL (can't access expiry directly, but expiry manager will remove after TTL) | |
| // Sleep slightly less than TTL and ensure key still exists | |
| Thread.sleep(1000); | |
| assertNotNull(db.getValue("ttl-list")); | |
| // Wait until after TTL (poll with timeout) | |
| long deadline = System.currentTimeMillis() + 6000; | |
| while (db.getValue("ttl-list") != null && System.currentTimeMillis() < deadline) { | |
| Thread.sleep(100); | |
| } | |
| assertNull(db.getValue("ttl-list")); |
🤖 Prompt for AI Agents
In `@src/test/java/com/redis/commands/LLenCommandTest.java` around lines 77 - 96,
The test testConcurrentSafePreserveTTLWithLPush uses fixed Thread.sleep calls
causing flakiness; replace them with bounded polling loops that repeatedly check
db.getValue("ttl-list") with short sleeps (e.g., 50–100ms) and a hard timeout
instead of the fixed Thread.sleep durations. Specifically, after calling
LPushCommand.execute(...) assert the key still exists by looping until either
db.getValue("ttl-list") != null or a 1.5s timeout elapses; then for the
post-expiry check loop until db.getValue("ttl-list") == null or a 3s timeout
elapses. Keep references to db.put(..., 2000), LPushCommand, and
db.getValue("ttl-list") so the replacement logic is applied in the same test
method.
| ByteBuf response1 = channel.readOutbound(); | ||
| ByteBuf response2 = channel.readOutbound(); | ||
|
|
||
| assertEquals("+PONG\r\n", response1.toString(StandardCharsets.UTF_8)); | ||
| assertEquals("+PONG\r\n", response2.toString(StandardCharsets.UTF_8), "Second pipelined command should be processed"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PipeliningTest.java" -type fRepository: unikdahal/redis-java
Length of output: 116
🏁 Script executed:
head -60 src/test/java/com/redis/server/PipeliningTest.javaRepository: unikdahal/redis-java
Length of output: 1848
🏁 Script executed:
wc -l src/test/java/com/redis/server/PipeliningTest.javaRepository: unikdahal/redis-java
Length of output: 117
🏁 Script executed:
find src/test -name "*.java" -type f | head -20Repository: unikdahal/redis-java
Length of output: 875
🏁 Script executed:
rg "EmbeddedChannel" src/test --type java -A 5 -B 2Repository: unikdahal/redis-java
Length of output: 2218
🏁 Script executed:
rg "ReferenceCountUtil|release\(" src/test --type java | head -20Repository: unikdahal/redis-java
Length of output: 46
🏁 Script executed:
cat pom.xml | grep -A 5 -B 5 "netty\|leak" | head -40Repository: unikdahal/redis-java
Length of output: 1399
🏁 Script executed:
rg "javaagent|ByteBufUtil|leak" src --type javaRepository: unikdahal/redis-java
Length of output: 46
🏁 Script executed:
find . -name "*.xml" -o -name "*.properties" | xargs grep -l "leak" 2>/dev/null | head -5Repository: unikdahal/redis-java
Length of output: 46
🏁 Script executed:
find . -name "*.xml" -o -name "*.gradle" -o -name "*.properties" | xargs grep -l "io.netty.leakDetection" 2>/dev/nullRepository: unikdahal/redis-java
Length of output: 48
🏁 Script executed:
rg "finalize|close\(" src/test --type java -B 2 -A 2 | head -40Repository: unikdahal/redis-java
Length of output: 46
🏁 Script executed:
python3 << 'EOF'
# Check Netty EmbeddedChannel documentation understanding
# Based on Netty 4.1.118.Final behavior
print("Netty EmbeddedChannel behavior:")
print("- EmbeddedChannel is a test utility for testing ChannelHandler")
print("- readOutbound() returns the next outbound message without removing it from the queue initially")
print("- ByteBuf objects returned have reference count >= 1")
print("- EmbeddedChannel.finishAndReleaseAll() releases all pending outbound messages")
print("- Individual ByteBuf objects should be released if not consuming them")
print("")
print("Leak detection:")
print("- Netty has ResourceLeakDetector which detects leaked ByteBuf instances")
print("- Leak detection is disabled by default in tests")
print("- Can be enabled via: -Dio.netty.leakDetection.level=paranoid")
print("- Tests should follow RAII pattern or explicitly release ByteBuf")
EOFRepository: unikdahal/redis-java
Length of output: 150
🏁 Script executed:
grep -r "finishAndReleaseAll" src --include="*.java"Repository: unikdahal/redis-java
Length of output: 46
🏁 Script executed:
# Check if EmbeddedChannel is closed in the tests
rg "channel\." src/test/java/com/redis/server/PipeliningTest.javaRepository: unikdahal/redis-java
Length of output: 540
🏁 Script executed:
# Check other Netty test patterns in the codebase
find src -name "*.java" -type f -path "*/test/*" -exec grep -l "ByteBuf\|EmbeddedChannel" {} \;Repository: unikdahal/redis-java
Length of output: 114
Release ByteBufs to avoid resource leaks in EmbeddedChannel tests.
The outbound ByteBuf objects from readOutbound() should be explicitly released. Without proper cleanup, Netty's EmbeddedChannel may hold references to these buffers, and the test methods don't invoke finishAndReleaseAll() or close() to ensure cleanup.
🧹 Release buffers
ByteBuf response1 = channel.readOutbound();
ByteBuf response2 = channel.readOutbound();
-
- assertEquals("+PONG\r\n", response1.toString(StandardCharsets.UTF_8));
- assertEquals("+PONG\r\n", response2.toString(StandardCharsets.UTF_8), "Second pipelined command should be processed");
+ try {
+ assertEquals("+PONG\r\n", response1.toString(StandardCharsets.UTF_8));
+ assertEquals("+PONG\r\n", response2.toString(StandardCharsets.UTF_8), "Second pipelined command should be processed");
+ } finally {
+ io.netty.util.ReferenceCountUtil.release(response1);
+ io.netty.util.ReferenceCountUtil.release(response2);
+ } ByteBuf finalResponse = channel.readOutbound();
- assertEquals("+PONG\r\n", finalResponse.toString(StandardCharsets.UTF_8));
+ try {
+ assertEquals("+PONG\r\n", finalResponse.toString(StandardCharsets.UTF_8));
+ } finally {
+ io.netty.util.ReferenceCountUtil.release(finalResponse);
+ }🤖 Prompt for AI Agents
In `@src/test/java/com/redis/server/PipeliningTest.java` around lines 25 - 29, The
test reads outbound ByteBufs (response1 and response2) from
channel.readOutbound() but never releases them; update the test to release each
ByteBuf after asserting (use response1.release()/response2.release() or
ReferenceCountUtil.release(...)) and/or call channel.finishAndReleaseAll() or
channel.close() in the test teardown to ensure EmbeddedChannel cleans up
resources; locate the usages of response1, response2, and channel.readOutbound()
in PipeliningTest and add the releases/final cleanup accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/redis/storage/RedisDatabase.java (1)
21-21: MissingvolatileonINSTANCEfield breaks double-checked locking.The double-checked locking pattern requires the singleton field to be
volatileto ensure proper visibility across threads. Without it, a thread could see a partially constructedRedisDatabaseinstance due to instruction reordering.Proposed fix
- private static RedisDatabase INSTANCE; + private static volatile RedisDatabase INSTANCE;Also applies to: 48-57
🤖 Fix all issues with AI agents
In `@src/main/java/com/redis/commands/BLPopCommand.java`:
- Around line 29-36: The Javadoc for execute() is currently appended to the
MAX_TIMEOUT_SECONDS declaration, so separate them by ending the
MAX_TIMEOUT_SECONDS declaration cleanly and move the multi-line Javadoc to
immediately precede the execute(...) method signature; specifically, remove the
Javadoc text from the same line as the MAX_TIMEOUT_SECONDS field, ensure
MAX_TIMEOUT_SECONDS remains a proper field (private static final long
MAX_TIMEOUT_SECONDS = 60 * 60 * 24;), and place the Javadoc block directly above
the execute(...) method to associate it with execute() instead of the field.
In `@src/main/java/com/redis/commands/PingCommand.java`:
- Around line 19-21: The RESP bulk string length is computed using Java char
count (msg.length()) which is incorrect for UTF-8; in the PingCommand code path
that builds the response (the block that checks args != null && args.size() == 1
and returns the "$...\\r\\n" string), replace the char-length call with the
UTF-8 byte length (e.g. use msg.getBytes(StandardCharsets.UTF_8).length) and
ensure StandardCharsets.UTF_8 is imported/used so multi-byte characters produce
the correct RESP length.
In `@src/main/java/com/redis/server/RedisCommandHandler.java`:
- Around line 147-157: The integer parsing in RedisCommandHandler (the block
using buf, rIndex, readerIndex, value and reading bytes b) currently ignores
non-digit characters and yields incorrect values; change the logic to validate
the entire token between the initial byte and the '\r' terminator: allow an
optional leading '-' then require every subsequent byte up to '\r' to be in
'0'..'9', and if any other byte is seen throw a protocol exception (e.g.,
ProtocolException or a Redis protocol error) instead of silently skipping;
update the initial branch that handles the first byte (b) to detect '-' and
digits and set a flag for negativity, and enforce digit-only bytes in the while
loop before converting to the integer value.
In `@src/main/test/integration_tests.sh`:
- Line 19: Replace the unquoted array assignment that risks
word-splitting/globbing: instead of using local args=($cmd) use Bash's read -ra
to parse the contents of the variable safely into the args array (e.g., read -ra
args <<< "$cmd"), ensuring the variable names cmd and args are used and quoting
the source "$cmd" so expansions are not subject to globbing or word splitting.
♻️ Duplicate comments (7)
src/main/java/com/redis/commands/PingCommand.java (1)
17-24: Missing validation for excess arguments.Redis
PINGaccepts at most one argument. When more than one argument is provided, Redis returns an error. Currently, multiple arguments silently fall through to returnPONG.Proposed fix
`@Override` public String execute(List<String> args, ChannelHandlerContext ctx) { // PING [message] - if message provided, echo it as bulk string + if (args != null && args.size() > 1) { + return "-ERR wrong number of arguments for 'ping' command\r\n"; + } if (args != null && args.size() == 1) { String msg = args.get(0); return "$" + msg.length() + "\r\n" + msg + "\r\n"; } return RESP_PONG; }src/main/java/com/redis/commands/EchoCommand.java (1)
20-21: Use byte length for RESP bulk strings.Line 21 uses
msg.length(), which counts UTF-16 code units, not bytes. Multi-byte UTF-8 input will corrupt RESP framing.Proposed fix
+import java.nio.charset.StandardCharsets; String msg = args.get(0); - return "$" + msg.length() + "\r\n" + msg + "\r\n"; + int byteLen = msg.getBytes(StandardCharsets.UTF_8).length; + return "$" + byteLen + "\r\n" + msg + "\r\n";run_all_tests.sh (1)
7-8: Make the script portable (avoid hard-coded path).Line 7 hard-codes a local path, which will fail on other machines or CI environments. Derive from the script location or allow override via environment variable.
Proposed fix
-PROJECT_DIR="/home/unik/Coding/backend/redis-java" +PROJECT_DIR="${PROJECT_DIR:-$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)}" cd "$PROJECT_DIR"src/main/java/com/redis/server/RedisCommandHandler.java (1)
82-125: Malformed RESP frames still cause connection stalls.The parsing logic returns
falsefor both incomplete and malformed input (e.g., line 88, 102). When malformed data arrives, the decoder resets the reader index and waits forever for "more data" that won't fix the protocol violation. Consider throwing an exception for definite protocol errors soexceptionCaughtcan close the channel.src/main/java/com/redis/commands/TtlCommand.java (1)
29-30: TTL arity validation still allows extra arguments.The validation only rejects empty args but accepts
TTL key extra_arg. Redis expects exactly one argument for TTL.🐛 Proposed fix
- if (args.isEmpty()) return ERR_WRONG_ARGS; + if (args.size() != 1) return ERR_WRONG_ARGS;src/main/java/com/redis/commands/BLPopCommand.java (2)
70-99:Thread.sleep()still blocks Netty's event loop.Based on learnings, command execution runs on Netty's single-threaded event loop per channel. The blocking
Thread.sleep()call freezes all client processing during the BLPOP wait period. This should be replaced with async scheduling usingctx.executor().schedule().
146-150: WRONGTYPE handling deviates from Redis specification.The comment on line 146-147 is incorrect. Per Redis documentation, BLPOP returns a WRONGTYPE error immediately upon encountering any key holding a non-list value—it does not skip to the next key. The current implementation silently ignores wrong-type keys.
🧹 Nitpick comments (3)
src/main/java/com/redis/commands/LPopCommand.java (1)
89-90: Prefer using the type-safeasList()accessor.
RedisValueprovides a type-safeasList()method that throwsIllegalStateExceptionon type mismatch. Since the type is already validated on line 84, you could simplify this and avoid the unchecked cast warning.♻️ Suggested simplification
- `@SuppressWarnings`("unchecked") - List<String> existingList = (List<String>) existing.getData(); + List<String> existingList = existing.asList();src/main/java/com/redis/commands/LLenCommand.java (1)
9-9: Minor formatting: missing space before{.Consider adding a space for consistency with Java style conventions.
♻️ Formatting fix
-public class LLenCommand implements ICommand{ +public class LLenCommand implements ICommand {src/main/java/com/redis/commands/BLPopCommand.java (1)
128-129: Prefer using the type-safeasList()accessor.Same as in
LPopCommand, you can useexisting.asList()instead of the unchecked cast since the type is already validated.♻️ Suggested simplification
- `@SuppressWarnings`("unchecked") - List<String> existingList = (List<String>) existing.getData(); + List<String> existingList = existing.asList();
| if (args != null && args.size() == 1) { | ||
| String msg = args.get(0); | ||
| return "$" + msg.length() + "\r\n" + msg + "\r\n"; |
There was a problem hiding this comment.
Use byte length for RESP bulk strings.
Line 21 uses msg.length(), which counts UTF-16 code units, not bytes. Multi-byte UTF-8 characters will produce incorrect RESP framing.
Proposed fix
+import java.nio.charset.StandardCharsets;
if (args != null && args.size() == 1) {
String msg = args.get(0);
- return "$" + msg.length() + "\r\n" + msg + "\r\n";
+ int byteLen = msg.getBytes(StandardCharsets.UTF_8).length;
+ return "$" + byteLen + "\r\n" + msg + "\r\n";
}🤖 Prompt for AI Agents
In `@src/main/java/com/redis/commands/PingCommand.java` around lines 19 - 21, The
RESP bulk string length is computed using Java char count (msg.length()) which
is incorrect for UTF-8; in the PingCommand code path that builds the response
(the block that checks args != null && args.size() == 1 and returns the
"$...\\r\\n" string), replace the char-length call with the UTF-8 byte length
(e.g. use msg.getBytes(StandardCharsets.UTF_8).length) and ensure
StandardCharsets.UTF_8 is imported/used so multi-byte characters produce the
correct RESP length.
| } else if (b >= '0' && b <= '9') { | ||
| value = b - '0'; | ||
| } | ||
|
|
||
| while (b != '\r') { | ||
| value = value * 10 + (b - '0'); | ||
| while (buf.readerIndex() <= rIndex) { | ||
| b = buf.readByte(); | ||
| if (b == '\r') break; | ||
| if (b >= '0' && b <= '9') { | ||
| value = value * 10 + (b - '0'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Non-digit characters in integer fields are silently ignored.
If malformed input contains non-digit characters (e.g., *abc\r\n), the parser silently ignores them and produces an incorrect result (0 in this case). Consider validating that all characters between the start and \r are digits (or a leading -), and throwing a protocol exception otherwise.
🐛 Suggested fix to validate integer format
while (buf.readerIndex() <= rIndex) {
b = buf.readByte();
if (b == '\r') break;
- if (b >= '0' && b <= '9') {
- value = value * 10 + (b - '0');
+ if (b < '0' || b > '9') {
+ throw new IllegalArgumentException("Malformed RESP: invalid integer");
}
+ value = value * 10 + (b - '0');
}📝 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.
| } else if (b >= '0' && b <= '9') { | |
| value = b - '0'; | |
| } | |
| while (b != '\r') { | |
| value = value * 10 + (b - '0'); | |
| while (buf.readerIndex() <= rIndex) { | |
| b = buf.readByte(); | |
| if (b == '\r') break; | |
| if (b >= '0' && b <= '9') { | |
| value = value * 10 + (b - '0'); | |
| } | |
| } | |
| } else if (b >= '0' && b <= '9') { | |
| value = b - '0'; | |
| } | |
| while (buf.readerIndex() <= rIndex) { | |
| b = buf.readByte(); | |
| if (b == '\r') break; | |
| if (b < '0' || b > '9') { | |
| throw new IllegalArgumentException("Malformed RESP: invalid integer"); | |
| } | |
| value = value * 10 + (b - '0'); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/redis/server/RedisCommandHandler.java` around lines 147 -
157, The integer parsing in RedisCommandHandler (the block using buf, rIndex,
readerIndex, value and reading bytes b) currently ignores non-digit characters
and yields incorrect values; change the logic to validate the entire token
between the initial byte and the '\r' terminator: allow an optional leading '-'
then require every subsequent byte up to '\r' to be in '0'..'9', and if any
other byte is seen throw a protocol exception (e.g., ProtocolException or a
Redis protocol error) instead of silently skipping; update the initial branch
that handles the first byte (b) to detect '-' and digits and set a flag for
negativity, and enforce digit-only bytes in the while loop before converting to
the integer value.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds significant new functionality to the Redis Java server by implementing 7 new commands (BLPOP, ECHO, PING, LPOP with count support, EXPIRE, TTL, LLEN) and refactoring the command handler to support pipelining and fragmentation. The changes also include critical bug fixes for expiry management and comprehensive test coverage.
Changes:
- Refactored
RedisCommandHandlerfromChannelInboundHandlerAdaptertoByteToMessageDecoderfor proper pipelining and fragmentation support - Added 7 new Redis commands with full unit test coverage (except PING and ECHO)
- Fixed critical expiry bugs by adding
clearExpiry()calls when keys are deleted or overwritten - Enhanced pom.xml with BOM-based dependency management and plugin version centralization
- Added comprehensive integration test suite with 200+ tests
- Improved documentation with detailed command reference and architecture diagrams
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/redis/server/RedisCommandHandler.java | Refactored to ByteToMessageDecoder for pipelining support |
| src/main/java/com/redis/storage/RedisDatabase.java | Added expiry management methods and clearExpiry() calls to fix bugs |
| src/main/java/com/redis/storage/ExpiryManager.java | Added clearExpiry() method for proper expiry cleanup |
| src/main/java/com/redis/commands/*.java | Added 7 new command implementations |
| src/test/java/com/redis/commands/*.java | Added unit tests for new commands |
| src/test/java/com/redis/storage/ExpiryBugTest.java | Regression tests for expiry bug fixes |
| src/test/java/com/redis/server/PipeliningTest.java | Tests for pipelining and fragmentation support |
| src/main/test/integration_tests.sh | Comprehensive integration test suite |
| run_all_tests.sh | Test runner script |
| pom.xml | Refactored with BOM management and updated versions |
| README.md | Enhanced documentation with badges, mermaid diagrams, and detailed command reference |
| build/classes/* | Build artifacts incorrectly committed to repository |
| <configuration> | ||
| <source>25</source> | ||
| <target>25</target> | ||
| <parameters>true</parameters> |
There was a problem hiding this comment.
The <parameters>true</parameters> configuration (line 133) enables the compiler to include parameter names in bytecode. While useful for debugging and reflection, it's not clear if this project needs this feature. If not required, consider removing it to slightly reduce JAR size and compilation time.
| <parameters>true</parameters> |
| // 4. Check if the key still exists. | ||
| // BUG: In the current implementation, it will be null because the background | ||
| // cleaner task for the first 'put' will see that keyExpiryMap still has | ||
| // the old expiry time (because it wasn't cleared) and remove the key. | ||
| assertEquals(val2, db.get(key), "Key should still exist after overwriting without TTL"); |
There was a problem hiding this comment.
The test comments at lines 27-29 describe a bug scenario but the test is not marked with @disabled or similar annotations. This is actually correct - the test is validating that the bug has been fixed by the changes in this PR (specifically the clearExpiry() calls added to RedisDatabase). However, the comment wording ("BUG: In the current implementation") suggests the bug still exists. Update the comment to clarify that this test validates the fix, e.g., "This test ensures that the expiry bug is fixed: when a key with TTL is overwritten without TTL, it should not expire."
| # Comprehensive integration tests for Redis Java server. | ||
| # Uses redis-cli style inline commands via netcat. | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
The script uses set -euo pipefail (line 5) which will cause the script to exit on any error. However, the test functions run_test and run_test_contains return non-zero on failure (lines 55, 90). With -e set, the first test failure will terminate the entire script instead of continuing to run remaining tests. Consider removing the -e flag or wrapping test calls with || true to allow all tests to run even if some fail.
| set -euo pipefail | |
| set -uo pipefail |
| - **💾 In-Memory Storage**: Optimized data structures using `ConcurrentHashMap` for thread-safe, lock-free reads. | ||
| - **🔌 Redis Protocol (RESP)**: Full support for the Redis Serialization Protocol, compatible with any standard Redis client (`redis-cli`, `jedis`, `redis-py`, etc.). | ||
| - **⏳ Advanced Expiration**: Dual-strategy expiration (Lazy + Active background cleanup via `DelayQueue`). | ||
| - **🎯 Single-Threaded Execution**: Mimics Redis's atomic command processing model for data consistency. |
There was a problem hiding this comment.
The claim "Mimics Redis's atomic command processing model for data consistency" (line 17) is misleading given that BLPOP blocks the I/O thread (as noted in comment ID 007). When BLPOP is executed, other clients cannot be served, which does NOT match Redis's behavior. Redis uses event-driven I/O to handle multiple blocking clients efficiently. Consider adding a caveat or removing this claim until BLPOP is refactored to use non-blocking I/O.
| - **🎯 Single-Threaded Execution**: Mimics Redis's atomic command processing model for data consistency. | |
| - **🎯 Single-Threaded Execution**: Primarily single-threaded command execution for predictable behavior; note that some blocking commands (e.g., `BLPOP`) currently block the I/O thread and do **not** yet match Redis's non-blocking event-driven handling. |
| // Poll until we find an element or timeout | ||
| while (System.currentTimeMillis() < deadline) { | ||
| // Try each key in order | ||
| for (String key : keys) { | ||
| String result = tryPopFromKey(db, key); | ||
| if (result != null) { | ||
| // Return [key, element] as RESP array | ||
| return formatResult(key, result); | ||
| } | ||
| } | ||
|
|
||
| // No element found, sleep before next poll | ||
| if (timeoutMs > 0) { | ||
| long remaining = deadline - System.currentTimeMillis(); | ||
| if (remaining <= 0) { | ||
| break; | ||
| } | ||
| long sleepTime = Math.min(POLL_INTERVAL_MS, remaining); | ||
| try { | ||
| Thread.sleep(sleepTime); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| return RESP_NIL; | ||
| } | ||
| } else { | ||
| // Zero timeout means block indefinitely - but check once | ||
| // For safety, we return nil immediately if nothing found with 0 timeout | ||
| // Real Redis would block forever, but that's not practical | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Timeout expired |
There was a problem hiding this comment.
The BLPOP implementation uses Thread.sleep() for blocking (lines 75-92), which blocks the Netty I/O thread. This is problematic because Netty's event loop is single-threaded and blocking it will prevent processing other clients' requests. Consider using Netty's ScheduledExecutorService or offloading blocking operations to a separate thread pool. The comment at line 89-90 acknowledges the issue with timeout=0 but doesn't address the broader blocking problem.
| // Poll until we find an element or timeout | |
| while (System.currentTimeMillis() < deadline) { | |
| // Try each key in order | |
| for (String key : keys) { | |
| String result = tryPopFromKey(db, key); | |
| if (result != null) { | |
| // Return [key, element] as RESP array | |
| return formatResult(key, result); | |
| } | |
| } | |
| // No element found, sleep before next poll | |
| if (timeoutMs > 0) { | |
| long remaining = deadline - System.currentTimeMillis(); | |
| if (remaining <= 0) { | |
| break; | |
| } | |
| long sleepTime = Math.min(POLL_INTERVAL_MS, remaining); | |
| try { | |
| Thread.sleep(sleepTime); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| return RESP_NIL; | |
| } | |
| } else { | |
| // Zero timeout means block indefinitely - but check once | |
| // For safety, we return nil immediately if nothing found with 0 timeout | |
| // Real Redis would block forever, but that's not practical | |
| break; | |
| } | |
| } | |
| // Timeout expired | |
| // Non-blocking behavior: try each key once and return immediately. | |
| // This avoids blocking the Netty I/O thread with Thread.sleep. | |
| for (String key : keys) { | |
| String result = tryPopFromKey(db, key); | |
| if (result != null) { | |
| // Return [key, element] as RESP array | |
| return formatResult(key, result); | |
| } | |
| } | |
| // No element available at this moment (or timeout semantics are not enforced to avoid blocking) |
| * Returns the command error reply {@code ERR_WRONG_ARGS} if fewer than two arguments are provided, | ||
| * or {@code ERR_VALUE} if the expiry value is not a valid integer. | ||
| * | ||
| * @param args the command arguments: {@code [key, seconds]} | ||
| * @param ctx the Netty channel context (not used by this implementation) | ||
| * @return {@code ":1\r\n"} if the expiry was set, {@code ":0\r\n"} otherwise | ||
| */ | ||
| @Override | ||
| public String execute(List<String> args, ChannelHandlerContext ctx) { |
There was a problem hiding this comment.
The EXPIRE command doesn't validate for negative seconds values. While Redis accepts negative values (which causes immediate expiration), this implementation passes negative seconds to System.currentTimeMillis() + (seconds * 1000L), resulting in a past timestamp. This may work correctly with the expiry system, but it's not explicitly tested or documented. Consider either rejecting negative values with an error or adding a comment explaining the behavior.
| public class RedisCommandHandler extends ByteToMessageDecoder { | ||
| // Preallocate list to avoid allocations for small commands | ||
| private static final int INITIAL_ARGS_CAPACITY = 16; | ||
| private final List<String> argsBuffer = new ArrayList<>(INITIAL_ARGS_CAPACITY); |
There was a problem hiding this comment.
The refactoring from ChannelInboundHandlerAdapter to ByteToMessageDecoder is a significant architectural change that improves pipelining and fragmentation support. However, there's a potential issue: the argsBuffer is now a class field (line 25) that's reused across calls. While decode() is called sequentially within a channel, ensure that this handler is not shared across multiple channels (which would cause thread-safety issues). Verify that the handler is instantiated per-channel in the server pipeline setup.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@unikdahal I've opened a new pull request, #43, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: unikdahal <61407386+unikdahal@users.noreply.github.com>
Co-authored-by: unikdahal <61407386+unikdahal@users.noreply.github.com>
[WIP] Refactor and add new commands for improved functionality
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| * EXPIRE key seconds [NX | XX | GT | LT] | ||
| * Set a timeout on key. After the timeout has expired, the key will automatically be deleted. | ||
| * | ||
| * Simple implementation supporting seconds. |
There was a problem hiding this comment.
The class javadoc on line 9 mentions that EXPIRE supports optional flags [NX | XX | GT | LT], but the implementation (line 31) does not validate or implement these flags. If more than 2 arguments are provided, they are silently ignored. The documentation should either be updated to reflect that these flags are not yet implemented, or the implementation should validate the argument count and return an error if unsupported flags are provided.
| # Clone and build (replace REPO_URL with the HTTPS or SSH URL of this repository or your fork) | ||
| git clone REPO_URL |
There was a problem hiding this comment.
The README states "Clone and build (replace REPO_URL with the HTTPS or SSH URL of this repository or your fork)" but the actual git clone command uses a placeholder "REPO_URL" that will fail if users copy-paste it directly. Consider either using the actual repository URL or providing a clearer example with a commented-out placeholder, such as: git clone <your-repo-url> or # git clone https://github.com/username/redis-java.git
| # Clone and build (replace REPO_URL with the HTTPS or SSH URL of this repository or your fork) | |
| git clone REPO_URL | |
| # Clone and build (replace <your-repo-url> with the HTTPS or SSH URL of this repository or your fork) | |
| git clone <your-repo-url> | |
| # Example: | |
| # git clone https://github.com/username/redis-java.git |
| assertEquals(RedisValue.Type.LIST, value.getType()); | ||
| assertEquals(1, value.asList().size()); | ||
| assertEquals("value1", value.asList().getFirst()); | ||
| assertEquals("value1", value.asList().get(0)); |
There was a problem hiding this comment.
The change from getFirst() to get(0) suggests this code was previously using Java 21+ SequencedCollection methods. Since the pom.xml now targets Java 17 (which doesn't have getFirst()), this change is necessary for compatibility. However, this is inconsistent with the README which still claims "JDK 25 or higher" is required. All such changes should be verified to work with Java 17, and the documentation should be updated accordingly.
| @@ -0,0 +1,147 @@ | |||
| #!/bin/bash | |||
| # Test Execution Script for Redis Java Integration Tests | |||
| # This script runs all test suites and generates a report | |||
There was a problem hiding this comment.
The shebang line uses #!/bin/bash but the script is executed with bash run_all_tests.sh in the execution context. For portability and correctness, ensure the script has execute permissions (chmod +x run_all_tests.sh) and can be invoked directly as ./run_all_tests.sh, or document that it should be run with bash run_all_tests.sh.
| # This script runs all test suites and generates a report | |
| # This script runs all test suites and generates a report | |
| # | |
| # Usage: | |
| # # Make the script executable and invoke via the shebang: | |
| # # chmod +x run_all_tests.sh | |
| # # ./run_all_tests.sh | |
| # # | |
| # # Or invoke explicitly with bash (as used in some environments): | |
| # # bash run_all_tests.sh |
| <java.version>17</java.version> | ||
| <maven.compiler.source>${java.version}</maven.compiler.source> | ||
| <maven.compiler.target>${java.version}</maven.compiler.target> |
There was a problem hiding this comment.
The PR description claims "Rewritten README with concise architecture, command reference, and usage guide" but doesn't mention the critical Java version downgrade from 25 to 17 in pom.xml. This is a significant breaking change that affects compatibility and may impact developers who have Java 25-specific code or dependencies. The PR description should explicitly mention this major configuration change.
| while (System.currentTimeMillis() < deadline) { | ||
| // Try each key in order | ||
| for (String key : keys) { | ||
| String result = tryPopFromKey(db, key); | ||
| if (result != null) { | ||
| // Return [key, element] as RESP array | ||
| return formatResult(key, result); | ||
| } | ||
| } | ||
|
|
||
| // No element found, sleep before next poll | ||
| if (timeoutMs > 0) { | ||
| long remaining = deadline - System.currentTimeMillis(); | ||
| if (remaining <= 0) { | ||
| break; | ||
| } | ||
| long sleepTime = Math.min(POLL_INTERVAL_MS, remaining); | ||
| try { | ||
| Thread.sleep(sleepTime); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| return RESP_NIL; | ||
| } |
There was a problem hiding this comment.
The BLPOP implementation uses Thread.sleep() which blocks the Netty I/O thread, preventing the server from processing other clients' requests during BLPOP execution. While there is a warning comment acknowledging this (lines 69-73), this is a critical operational issue that makes the implementation unsuitable for production use. The comment suggests using "Netty's event loop scheduling or a separate executor thread pool", which should be implemented before merging this blocking implementation. This blocking behavior violates Redis's asynchronous architecture and will cause severe performance degradation under load.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
|
||
| ### Running the Server | ||
| ### Prerequisites | ||
| - **JDK 25** or higher (with `--enable-preview`) |
There was a problem hiding this comment.
The README states "JDK 25 or higher (with --enable-preview)" is required on line 54, but the pom.xml has been changed to target Java 17 (line 12). This creates a critical inconsistency - developers following the README will install Java 25 unnecessarily, while the actual build configuration targets Java 17. Either revert the pom.xml changes to use Java 25 with preview features as originally intended, or update this line to read "JDK 17 or higher".
| - **JDK 25** or higher (with `--enable-preview`) | |
| - **JDK 17** or higher |
|
|
||
| ```sh | ||
| # Start the server | ||
| java --enable-preview -jar target/redis-server.jar |
There was a problem hiding this comment.
The command java --enable-preview -jar target/redis-server.jar uses the --enable-preview flag, but since the pom.xml now targets Java 17 (which doesn't require or support preview features in this context), this flag is unnecessary and may cause confusion. Either remove the flag from this command or revert to Java 25 configuration in pom.xml.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@unikdahal I've opened a new pull request, #44, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…leep Co-authored-by: unikdahal <61407386+unikdahal@users.noreply.github.com>
[WIP] WIP to address feedback on refactored commands PR
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.