Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use concurrent skiplist map for NettyRequest arguments #2927

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Use concurrent skiplist map for NettyRequest arguments #2927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ambry-rest/src/main/java/com/github/ambry/rest/NettyRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
import java.util.Map;
import java.util.Queue;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentSkipListMap;
import java.util.concurrent.Future;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.atomic.AtomicBoolean;
Expand Down Expand Up @@ -69,7 +69,10 @@ public class NettyRequest implements RestRequest {
protected final HttpRequest request;
protected final Channel channel;
protected final NettyMetrics nettyMetrics;
protected final Map<String, Object> allArgs = new ConcurrentHashMap<>();
// This allArgs map needs to be
// 1. case-insensitive, as headers and query parameters in http request should be case-insensitive
// 2. thread safe, as we might access/update the map in different threads.
protected final Map<String, Object> allArgs = new ConcurrentSkipListMap<>(String.CASE_INSENSITIVE_ORDER);
protected final Queue<HttpContent> requestContents = new LinkedBlockingQueue<>();
protected final ReentrantLock contentLock = new ReentrantLock();

Expand Down
11 changes: 9 additions & 2 deletions ambry-rest/src/test/java/com/github/ambry/rest/NettyRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,15 @@ private void validateRequest(NettyRequest nettyRequest, RestMethod restMethod, S

for (Map.Entry<String, String> e : headers) {
if (!e.getKey().equalsIgnoreCase(HttpHeaderNames.COOKIE.toString())) {
// Make sure we have this key in receivedArgs
assertTrue("Did not find key: " + e.getKey(), receivedArgs.containsKey(e.getKey()));
// Now make sure we can find a case-insensitive key in the request
String lowerKey = e.getKey().toLowerCase();
assertTrue("Case insensitive key should exist for lower key" + e.getKey(),
nettyRequest.getArgs().containsKey(lowerKey));
String upperKey = e.getKey().toUpperCase();
assertTrue("Case insensitive key should exist for upper key" + e.getKey(),
nettyRequest.getArgs().containsKey(upperKey));
if (!keyValueCount.containsKey(e.getKey())) {
keyValueCount.put(e.getKey(), 0);
}
Expand All @@ -769,8 +777,7 @@ private void validateRequest(NettyRequest nettyRequest, RestMethod restMethod, S
assertEquals("Number of args does not match", keyValueCount.size(), receivedArgs.size());
for (Map.Entry<String, Integer> e : keyValueCount.entrySet()) {
assertEquals("Value count for key " + e.getKey() + " does not match",
e.getValue() == 0 ? 1 : e.getValue().intValue(),
receivedArgs.get(e.getKey()).size());
e.getValue() == 0 ? 1 : e.getValue().intValue(), receivedArgs.get(e.getKey()).size());
}

assertEquals("Auto-read is in an invalid state",
Expand Down
Loading