esevastyanov commented on a change in pull request #8157: Enum of ResponseContext keys URL: https://github.com/apache/incubator-druid/pull/8157#discussion_r307880724
########## File path: processing/src/main/java/org/apache/druid/query/context/ResponseContext.java ########## @@ -76,56 +138,128 @@ public static ResponseContext createEmpty() return DefaultResponseContext.createEmpty(); } - protected abstract Map<String, Object> getDelegate(); - - public Object put(String key, Object value) + public static ResponseContext deserialize(String responseContext, ObjectMapper objectMapper) throws IOException { - return getDelegate().put(key, value); + final Map<String, Object> delegate = objectMapper.readValue( + responseContext, + JacksonUtils.TYPE_REFERENCE_MAP_STRING_OBJECT + ); + return new ResponseContext() + { + @Override + protected Map<String, Object> getDelegate() + { + return delegate; + } + }; } - public Object get(String key) + protected abstract Map<String, Object> getDelegate(); + + public Object put(Key key, Object value) { - return getDelegate().get(key); + return getDelegate().put(key.name, value); } - public Object remove(String key) + public Object get(Key key) { - return getDelegate().remove(key); + return getDelegate().get(key.name); } - public void putAll(Map<? extends String, ?> m) + public Object remove(Key key) { - getDelegate().putAll(m); + return getDelegate().remove(key.name); } - public void putAll(ResponseContext responseContext) + /** + * Merges a new value associated with a key with an old value. + */ + public Object merge(Key key, Object value) { - getDelegate().putAll(responseContext.getDelegate()); + return getDelegate().merge(key.name, value, key.mergeFunction); } - public int size() + /** + * Merges a response context into current. + * This method merges only keys from the enum {@link Key}. + */ + public void merge(ResponseContext responseContext) { - return getDelegate().size(); + for (Key key : Key.values()) { + final Object newValue = responseContext.get(key); + if (newValue != null) { + merge(key, newValue); + } + } } - public String serializeWith(ObjectMapper objectMapper) throws JsonProcessingException + /** + * Serializes the context given that the resulting string length is less than the provided limit. + * The method removes max-length fields one by one if the resulting string length is greater than the limit. + * The resulting string might be correctly deserialized as a {@link ResponseContext}. Review comment: Previously the serialized value was literally truncated (hence non-deserializable) thus, in my opinion, there is no correct solution on how to reduce the size of the serialized value. + Manually specified priorities of keys. Ideally, this method requires some research on how context values are used by users (because it is a part of the response). E.g. from codebase perspective, it's known that `Etag` key is for caching results as a part HTTP protocol. However, it's not obvious how other keys (values) are used. + Reducing the value of specific keys. Thought about this approach and it seems close to the ideal solution. The only issue is it requires additional information about values (similar to merge function) and much more iterations on reduce phase (in case of using algorithm analogous to the current). In my view, both of these approaches look as premature optimization without understanding the scope of the issue. ---------------------------------------------------------------- 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 --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org