leventov commented on a change in pull request #8157: Enum of ResponseContext keys URL: https://github.com/apache/incubator-druid/pull/8157#discussion_r308791241
########## 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: 1) Please put this discussion in code comments. 2) I see a regression scenario: before this PR, `UNCOVERED_INTERVALS` and `MISSING_SEGMENTS` keys were always reasonably short. Now, they may grow very large at Broker, and Broker will prune them altogether. I suggest to hard-code reduction logic specifically for `UNCOVERED_INTERVALS` and `MISSING_SEGMENTS`. ---------------------------------------------------------------- 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