[GitHub] [helix] dasahcc commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
dasahcc commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r310212588
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##
@@ -97,7 +106,43 @@ public CustomRestClientImpl(HttpClient httpClient) {
kv -> result.put(kv.getKey(),
kv.getValue().get(IS_HEALTHY_FIELD).asBoolean()));
return result;
};
-return handleResponse(post(url, payLoads), jsonConverter);
+String requestId = baseUrl + customPayloads.toString() +
partitions.toString();
+try {
+ lock(requestId);
+ if (_cachedResults.containsKey(requestId)) {
+return _cachedResults.get(requestId);
+ }
+ Map result = handleResponse(post(url, payLoads),
jsonConverter);
+ _cachedResults.put(requestId, result);
+ return result;
+} catch (InterruptedException e) {
+ e.printStackTrace();
Review comment:
Why not use log but print out?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
dasahcc commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r310212407
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##
@@ -97,7 +106,43 @@ public CustomRestClientImpl(HttpClient httpClient) {
kv -> result.put(kv.getKey(),
kv.getValue().get(IS_HEALTHY_FIELD).asBoolean()));
return result;
};
-return handleResponse(post(url, payLoads), jsonConverter);
+String requestId = baseUrl + customPayloads.toString() +
partitions.toString();
+try {
+ lock(requestId);
+ if (_cachedResults.containsKey(requestId)) {
+return _cachedResults.get(requestId);
+ }
+ Map result = handleResponse(post(url, payLoads),
jsonConverter);
+ _cachedResults.put(requestId, result);
+ return result;
+} catch (InterruptedException e) {
+ e.printStackTrace();
+} finally {
+ unlock(requestId);
+}
+return Collections.emptyMap();
+ }
+
+ @VisibleForTesting
+ Map> getCachedResults() {
+return _cachedResults;
+ }
+
+ private void lock(String key) throws InterruptedException {
+synchronized (lockedKeys) {
Review comment:
This is not locking on key level but set object level.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
dasahcc commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r309831540
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##
@@ -52,16 +56,26 @@
private static final String ACCEPT_CONTENT_TYPE = "application/json";
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
- private HttpClient _httpClient;
+ private final HttpClient _httpClient;
+ private final Semaphore _semaphore;
+ // a transient cache holds the HttpResponse of HttpPosts, only valid when
cache is enabled
+ private final Map _cachedResults = new HashMap<>();
private interface JsonConverter {
Map convert(JsonNode jsonNode);
}
- public CustomRestClientImpl(HttpClient httpClient) {
+ CustomRestClientImpl(HttpClient httpClient) {
_httpClient = httpClient;
+_semaphore = new Semaphore(10);
Review comment:
@i3wangyi It should be one, right?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
dasahcc commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r309467486
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##
@@ -122,18 +144,23 @@ protected JsonNode getJsonObject(HttpResponse
httpResponse) throws IOException {
@VisibleForTesting
protected HttpResponse post(String url, Map payloads) throws
IOException {
+HttpPost postRequest = new HttpPost(url);
+postRequest.setHeader("Accept", ACCEPT_CONTENT_TYPE);
+StringEntity entity = new
StringEntity(OBJECT_MAPPER.writeValueAsString(payloads),
ContentType.APPLICATION_JSON);
+postRequest.setEntity(entity);
+HttpResponse httpResponse;
+int hashCode = url.hashCode() + payloads.hashCode();
try {
- HttpPost postRequest = new HttpPost(url);
- postRequest.setHeader("Accept", ACCEPT_CONTENT_TYPE);
- StringEntity entity = new
StringEntity(OBJECT_MAPPER.writeValueAsString(payloads),
- ContentType.APPLICATION_JSON);
- postRequest.setEntity(entity);
+ if (_cachedResults.containsKey(hashCode)) {
Review comment:
Leave a TODO comments here. It is better to have response data processed
after caching. Otherwise, every single call still needs to process the
responses.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
dasahcc commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r309466643
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##
@@ -28,39 +28,36 @@
import org.slf4j.LoggerFactory;
/**
- * The memory efficient factory to create instances for {@link
CustomRestClient}
+ * The memory efficient factory to getHttpClient instances for {@link
CustomRestClient}
*/
public class CustomRestClientFactory {
private static final Logger LOG =
LoggerFactory.getLogger(CustomRestClientFactory.class);
- private static CustomRestClient INSTANCE = null;
+ private static HttpClient HTTP_CLIENT_INSTANCE = null;
private CustomRestClientFactory() {
}
public static CustomRestClient get() {
Review comment:
Add some comments here to remind the CustomRestClient is per request client.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
dasahcc commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r309455471
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##
@@ -97,7 +111,15 @@ public CustomRestClientImpl(HttpClient httpClient) {
kv -> result.put(kv.getKey(),
kv.getValue().get(IS_HEALTHY_FIELD).asBoolean()));
return result;
};
-return handleResponse(post(url, payLoads), jsonConverter);
+try {
+ _semaphore.acquire();
+ return handleResponse(post(url, payLoads), jsonConverter);
+} catch (InterruptedException e) {
+ LOG.warn("Interrupted while acquiring the semaphore", e);
+} finally {
+ _semaphore.release();
Review comment:
That's not the method to lock, right? This is per instance check, it can be
parallel without duplication. The most duplicated requests part is partition
level checks.
Another thing here, shall we use Semaphore or we can use conditional
variable?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
dasahcc commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r308992497
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##
@@ -121,19 +125,24 @@ protected JsonNode getJsonObject(HttpResponse
httpResponse) throws IOException {
}
@VisibleForTesting
- protected HttpResponse post(String url, Map payloads) throws
IOException {
+ protected synchronized HttpResponse post(String url, Map
payloads) throws IOException {
Review comment:
You will have problem with locking on this method. It even can blocks per
node querying. I think we should lock inside get per partition data.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
