#25: Set up MangoTree to register and route PUT commands #26
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new server application, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MangoTree
participant CommandParser
participant ServerRegistry
participant MangoClient
participant ReplicationManager
Client->>MangoTree: Connects and sends command
MangoTree->>CommandParser: Parses command
CommandParser-->>MangoTree: Returns Command object
alt REGISTER command
MangoTree->>ServerRegistry: Register server with role, host, port
ServerRegistry-->>MangoTree: Update registry
MangoTree-->>Client: Respond with registration status
else HEARTBEAT command
MangoTree->>ServerRegistry: Update server heartbeat
ServerRegistry-->>MangoTree: Update status
MangoTree-->>Client: Respond with heartbeat acknowledgement
else SECONDARIES command
MangoTree->>ServerRegistry: Get list of secondaries
ServerRegistry-->>MangoTree: List of secondary servers
MangoTree-->>Client: Return secondary server addresses
else PUT command
MangoTree->>MangoClient (PRIMARY): Forward PUT command
MangoClient-->>MangoTree: Return response
MangoTree->>ReplicationManager: asyncReplicate(key, value)
MangoTree-->>Client: Return response
else GET command
MangoTree->>MangoClient (random SECONDARY): Forward GET command
MangoClient-->>MangoTree: Return response
MangoTree-->>Client: Return response
else DELETE command
MangoTree->>MangoClient (PRIMARY): Forward DELETE command
MangoClient-->>MangoTree: Return response
MangoTree-->>Client: Return response
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/main/java/commands/CommandParser.java (1)
59-67: Simplify arity check & avoid duplicated conditions
if (args.length < REGISTER.getParameterCount() || args.length > REGISTER.getParameterCount())is equivalent to!=. Using!=communicates intent better and avoids 2 comparisons.-if (args.length < REGISTER.getParameterCount() || args.length > REGISTER.getParameterCount()) { +if (args.length != REGISTER.getParameterCount()) {src/main/java/tree/MangoTree.java (1)
101-109: Socket is served in clear-text – consider TLSStatic analysis flagged the use of
java.net.ServerSocketwithout encryption. If any sensitive data or credentials travel through this channel, switch toSSLServerSocketor wrap the stream in TLS (e.g., with Netty/SSLEngine) to prevent MITM attacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/commands/CommandParser.java(2 hunks)src/main/java/commands/CommandType.java(1 hunks)src/main/java/tree/MangoTree.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
src/main/java/tree/MangoTree.java
[info] 100-100: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
| HELP(0, "HELP expects no parameters"), | ||
| HEARTBEAT(2, "ERROR: Usage: HEARTBEAT <HOST> <PORT>"), | ||
| REGISTER(3, "ERROR: Usage: REGISTER <PRIMARY|SECONDARY> <HOST> <PORT>"); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
HEARTBEAT parameter count conflicts with its actual parser handling
HEARTBEAT is declared with parameterCount = 2, yet CommandParser.handleHeartbeat() accepts zero arguments and does not validate the incoming argsString. This mismatch means:
- A syntactically-correct
HEARTBEAT <HOST> <PORT>command will be parsed as invalid (args discarded), whilst - A bare
HEARTBEAT(no args) will happily pass, breaking the public contract advertised by the error message.
Update either the enum or the parser so they agree on the same arity; otherwise every client/server pairing will eventually drift into inconsistent behaviour.
- HEARTBEAT(2, "ERROR: Usage: HEARTBEAT <HOST> <PORT>"),
+ HEARTBEAT(0, "HEARTBEAT expects no parameters"),#!/bin/bash
# Show CommandType enum around HEARTBEAT
rg -n -C2 "HEARTBEAT" src/main/java/commands/CommandType.java
# Inspect handleHeartbeat in CommandParser
rg -n -C10 "handleHeartbeat" src/main/java/commands/CommandParser.java
# Search where parameterCount is used in CommandParser
rg -n -C3 "parameterCount" src/main/java/commands/CommandParser.java#!/bin/bash
# Inspect the HEARTBEAT enum entry
rg -n -C2 "HEARTBEAT" src/main/java/commands/CommandType.java
# Inspect handleHeartbeat method in CommandParser
rg -n -C5 "handleHeartbeat" src/main/java/commands/CommandParser.java
# Find where parameterCount from CommandType is enforced or checked
rg -n -C5 "parameterCount" src/main/java/commands/CommandParser.java🏁 Script executed:
#!/bin/bash
# Show the HEARTBEAT entry in CommandType
rg -n -C2 "HEARTBEAT" src/main/java/commands/CommandType.java
# Show the handleHeartbeat implementation in CommandParser
rg -n -C5 "handleHeartbeat" src/main/java/commands/CommandParser.java
# Show where CommandType.parameterCount is enforced in CommandParser
rg -n -C5 "parameterCount" src/main/java/commands/CommandParser.javaLength of output: 1258
Align HEARTBEAT’s declared arity with its parser
The enum entry for HEARTBEAT declares parameterCount = 2 with a usage message for <HOST> <PORT>, but CommandParser.handleHeartbeat() ignores argsString entirely and always returns a zero-arg command. This mismatch will:
- Reject valid
HEARTBEAT <HOST> <PORT>invocations (args dropped) - Accept a bare
HEARTBEATwithout error
Choose one consistent approach:
- Parse two parameters in
handleHeartbeat(...), validateargsString.split(" ")length == 2, and pass host/port into the newCommand(HEARTBEAT, args) - Or change the enum to
parameterCount = 0and update the message to"HEARTBEAT expects no parameters"
Locations to update:
src/main/java/commands/CommandType.java(line 11)src/main/java/commands/CommandParser.java(handleHeartbeatat lines 69–71)
📝 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.
| HELP(0, "HELP expects no parameters"), | |
| HEARTBEAT(2, "ERROR: Usage: HEARTBEAT <HOST> <PORT>"), | |
| REGISTER(3, "ERROR: Usage: REGISTER <PRIMARY|SECONDARY> <HOST> <PORT>"); | |
| HELP(0, "HELP expects no parameters"), | |
| HEARTBEAT(0, "HEARTBEAT expects no parameters"), | |
| REGISTER(3, "ERROR: Usage: REGISTER <PRIMARY|SECONDARY> <HOST> <PORT>"); |
| case REGISTER -> handleRegister(argsString); | ||
| }; | ||
| } | ||
|
|
||
| private static Command handleRegister(final String argsString) { | ||
| String[] args = argsString.strip().split(SPACE_DELIMITER); | ||
|
|
||
| if (args.length < REGISTER.getParameterCount() || args.length > REGISTER.getParameterCount()) { | ||
| throw new InvalidParametersException(REGISTER.getErrorMessage()); | ||
| } | ||
|
|
||
| return new Command(REGISTER, args); | ||
| } | ||
|
|
||
| private static Command handleHeartbeat(final String argsString) { | ||
| return new Command(HEARTBEAT, null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent HEARTBEAT handling & missing validation
handleHeartbeat() discards argsString entirely. If the enum is kept at 2 params, this method should validate arity exactly as the other handlers do; if the enum is changed to 0, the error message inside the enum should be updated accordingly (see previous comment).
-private static Command handleHeartbeat(final String argsString) {
- return new Command(HEARTBEAT, null);
+private static Command handleHeartbeat(final String argsString) {
+ if (!argsString.isEmpty()) {
+ throw new InvalidParametersException(HEARTBEAT.getErrorMessage());
+ }
+ return new Command(HEARTBEAT, null);
}📝 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.
| case REGISTER -> handleRegister(argsString); | |
| }; | |
| } | |
| private static Command handleRegister(final String argsString) { | |
| String[] args = argsString.strip().split(SPACE_DELIMITER); | |
| if (args.length < REGISTER.getParameterCount() || args.length > REGISTER.getParameterCount()) { | |
| throw new InvalidParametersException(REGISTER.getErrorMessage()); | |
| } | |
| return new Command(REGISTER, args); | |
| } | |
| private static Command handleHeartbeat(final String argsString) { | |
| return new Command(HEARTBEAT, null); | |
| } | |
| private static Command handleHeartbeat(final String argsString) { | |
| if (!argsString.isEmpty()) { | |
| throw new InvalidParametersException(HEARTBEAT.getErrorMessage()); | |
| } | |
| return new Command(HEARTBEAT, null); | |
| } |
| if (role == ServerRole.PRIMARY) { | ||
| if (primaryServerId != null && primaryServerId != serverId) { | ||
| throw new RuntimeException("There's already a primary server registered"); | ||
| } | ||
| } |
There was a problem hiding this comment.
String comparison uses != instead of .equals
primaryServerId != serverId compares references, not content. Use !primaryServerId.equals(serverId) to avoid false positives/negatives.
-if (primaryServerId != null && primaryServerId != serverId) {
+if (primaryServerId != null && !primaryServerId.equals(serverId)) {📝 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.
| if (role == ServerRole.PRIMARY) { | |
| if (primaryServerId != null && primaryServerId != serverId) { | |
| throw new RuntimeException("There's already a primary server registered"); | |
| } | |
| } | |
| if (role == ServerRole.PRIMARY) { | |
| if (primaryServerId != null && !primaryServerId.equals(serverId)) { | |
| throw new RuntimeException("There's already a primary server registered"); | |
| } | |
| } |
| private HashMap<String, ServerInfo> registeredServers; | ||
| private String primaryServerId; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
HashMap is not thread-safe for concurrent connection threads
registeredServers is accessed and mutated from every connectionExecutor thread. A plain HashMap will race, leading to visibility issues or ConcurrentModificationException. Replace with a concurrent collection or introduce proper synchronisation.
-private HashMap<String, ServerInfo> registeredServers;
+private final ConcurrentMap<String, ServerInfo> registeredServers = new ConcurrentHashMap<>();📝 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 HashMap<String, ServerInfo> registeredServers; | |
| private String primaryServerId; | |
| private final ConcurrentMap<String, ServerInfo> registeredServers = new ConcurrentHashMap<>(); | |
| private String primaryServerId; |
src/main/java/tree/MangoTree.java
Outdated
| if (command.type().equals(CommandType.REGISTER)) { | ||
| System.out.println("Registering command: " + command); | ||
| String role = command.args()[0]; | ||
| Integer port = Integer.valueOf(command.args()[1]); | ||
| String host = command.args()[2]; | ||
|
|
||
| try { | ||
| handleRegistration(role, port, host); | ||
| } catch (final Exception e) { | ||
| System.err.println("Error registering command: " + e.getMessage()); | ||
| return String.format(WRAP_RED, "ERROR REGISTERING: " + e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| return String.format(WRAP_GREEN, "SUCCESSFULLY REGISTERED"); |
There was a problem hiding this comment.
Argument order for REGISTER is reversed – will throw NumberFormatException
Specification: REGISTER <PRIMARY|SECONDARY> <HOST> <PORT>
Code: role = args[0]; port = Integer.valueOf(args[1]); host = args[2];
args[1] is <HOST> (e.g. "127.0.0.1") – parsing it as an Integer will explode the first time a client registers. Swap the indices or change the spec.
-String role = command.args()[0];
-Integer port = Integer.valueOf(command.args()[1]);
-String host = command.args()[2];
+String role = command.args()[0];
+String host = command.args()[1];
+Integer port = Integer.valueOf(command.args()[2]);📝 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.
| if (command.type().equals(CommandType.REGISTER)) { | |
| System.out.println("Registering command: " + command); | |
| String role = command.args()[0]; | |
| Integer port = Integer.valueOf(command.args()[1]); | |
| String host = command.args()[2]; | |
| try { | |
| handleRegistration(role, port, host); | |
| } catch (final Exception e) { | |
| System.err.println("Error registering command: " + e.getMessage()); | |
| return String.format(WRAP_RED, "ERROR REGISTERING: " + e.getMessage()); | |
| } | |
| } | |
| return String.format(WRAP_GREEN, "SUCCESSFULLY REGISTERED"); | |
| if (command.type().equals(CommandType.REGISTER)) { | |
| System.out.println("Registering command: " + command); | |
| - String role = command.args()[0]; | |
| - Integer port = Integer.valueOf(command.args()[1]); | |
| - String host = command.args()[2]; | |
| + String role = command.args()[0]; | |
| + String host = command.args()[1]; | |
| + Integer port = Integer.valueOf(command.args()[2]); | |
| try { | |
| handleRegistration(role, port, host); | |
| } catch (final Exception e) { | |
| System.err.println("Error registering command: " + e.getMessage()); | |
| return String.format(WRAP_RED, "ERROR REGISTERING: " + e.getMessage()); | |
| } | |
| } | |
| return String.format(WRAP_GREEN, "SUCCESSFULLY REGISTERED"); |
| public void stop() { | ||
| // stop health check executor | ||
| // stop connection executor | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
stop() is empty – resources are never released
start() calls stop() in the finally block, but stop() does nothing. At minimum, shut down executors and close all client sockets to avoid thread leaks on server restart/shutdown.
public void stop() {
- // stop health check executor
- // stop connection executor
+ connectionExecutor.shutdownNow();
+ healthCheckExecutor.shutdownNow();
+ registeredServers.values().forEach(s -> {
+ try { s.getClient().close(); } catch (IOException ignored) {}
+ });
}📝 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 void stop() { | |
| // stop health check executor | |
| // stop connection executor | |
| } | |
| public void stop() { | |
| connectionExecutor.shutdownNow(); | |
| healthCheckExecutor.shutdownNow(); | |
| registeredServers.values().forEach(s -> { | |
| try { | |
| s.getClient().close(); | |
| } catch (IOException ignored) { | |
| } | |
| }); | |
| } |
Summary by CodeRabbit