paul-rogers opened a new pull request #11828:
URL: https://github.com/apache/druid/pull/11828


   ### Description
   
   Fixes a number of annoying issues in preparation for request trailers and 
the query profile.
   
   * Converts keys from an enum to classes for smaller code
   * Wraps stored values in functions for easier capture for other uses
   * Reworks the "header squeezer" to handle types other than arrays.
   * Uses metadata for visibility, and ability to compress, to replace ad-hoc 
code.
   * Other miscellaneous cleanup.
   
   Based on a prototype by Gian.
   
   #### Preparing for response trailers and query profile
   
   The query profile work needs a way to return profile information at the 
completion of a query. It turns out Gian had prototyped just such a mechanism. 
To do that, he modified the response context to make it clear which fields are 
returned in the trailer vs. the header. vs. not at all.
   
   The query profile will use the response context as a "back door" to sneak 
metrics information out of each `QueryRunner.run()` call. This PR contains just 
the refactoring done by those projects without the additional complexity of the 
actual trailer or profile code.
   
   #### Key metadata
   
   `ResponseContext` provides a set of key values that define the values 
allowed in the the context. It seems that those values were originally for a 
small set of request header values, but grew over time to include internal-only 
values. With the addition of the response trailer (coming in a later PR), we'll 
also have footer values.
   
   We have code that knows which names should be in the header and picks those 
out. However, extensions can add keys, which makes a hard-code approach 
brittle. This PR adds metadata to each key to identify its visibility. (Based 
on work originally done by Gian.)
   
   #### Expanded header truncation support
   
   The code can drop (or truncate) header fields. The code hard-codes those 
fields that can be truncated, and assumes they are arrays. This PR adds the 
"droppable" state as metadata in the key, and handles non-array fields when 
dropping. This ensures that extensions get the same "droppable" behavior as 
core code.
   
   #### Keys as classes, not enums
   
   Keys are defined as an enum. Enums define a fixed set of values. Yet, we 
allow extensions to "extend" the set of keys. In this case, it is easier to 
define keys as objects not in an enum.
   
   With this move, we can simplify key definitions. Enum values can't be 
instances of a class. But, with the additional responsibilities placed on keys, 
the enum structured forced quite a bit of redundant code. Changing keys to 
objects let's us use the usual code reuse techniques to avoid redundancy.
   
   #### Explicit accessors
   
   The code to use the response context usually looks like this:
   
   ```java
   responseContext.add(ResponseContext.Key.CPU_CONSUMED_NANOS, cpuTimeNs);
   ```
   
   That is rather verbose. And, if we wanted to capture the value for use in, 
say, metrics, we'd have to hunt down all the places that update the response 
context and add code to update metrics. This PR provides accessors instead:
   
   ```java
   responseContext.addCpuNanos(cpuTimeNs);
   ```
   
   #### Extension impact
   
   The prior version of the response context suggested that keys be defined in 
an enum, then registered. This version suggests that keys be defined as 
objects, then registered. This new version *may* require changes to extensions 
to adopt the new metadata format. However, such changes should be simple.
   
   ### Summary
   
   This PR has:
   
   - [X] been self-reviewed.
   - [X] added documentation for new or modified features or behaviors.
   - [X] added Javadocs for most classes and all non-trivial methods. Linked 
related entities via Javadoc links.
   - [X] added comments explaining the "why" and the intent of the code 
wherever would not be obvious for an unfamiliar reader.
   - [X] added unit tests or modified existing tests to cover new code paths, 
ensuring the threshold for [code 
coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md)
 is met.
   - [X] been tested in a local Druid cluster.
   


-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to