[DO NOT MERGE] Feature/master/fix query perf - #7370
Open
alextwoods wants to merge 8 commits into
Open
Conversation
The previous commit removed HTTP_REQUEST_URI_BEFORE_MODIFY outright, which broke customers who had started reading it despite it being internal API. Restore it as a deprecated derived view over a new HTTP_REQUEST_BEFORE_MODIFY attribute, which holds the marshalled request itself. Derived attributes apply their read mapping on each read, so the URI is only built if something actually asks for it, and the value is identical to before including the query string. EndpointResolutionStage reads the endpoint components straight off the snapshotted request, so the request path no longer builds a URI at all. This keeps the query/ec2 protocol regression fixed: those protocols still carry the entire payload in the raw query parameters at this point in the execution, so building a URI cost two passes over the whole payload per API call.
There is no write usage of this attribute, so setting it now throws UnsupportedOperationException rather than projecting the URI back onto the snapshotted request. This matches UnmodifiableExecutionAttributes, and is safe because ExecutionAttributes copies, merges and putAbsentAttributes all duplicate the backing map directly and never invoke a derived attribute's write mapping.
Holding the marshalled request kept its raw query parameters reachable until the end of the API call, because the signing stage swaps the interceptor context over to the signed request and LowCopyListMap#clear installs a fresh map rather than mutating the original. For query and ec2 that map is the request payload: measured at ~114KB retained for a 33KB payload, against ~69KB for the URI that 2.47.0 retained. Snapshot only the endpoint components instead, which retains ~130 bytes. The deprecated HTTP_REQUEST_URI_BEFORE_MODIFY becomes a view over those components, so it now carries no query string and always renders the port. The only known consumer parses the path for a "/invocations" suffix, which is unaffected; EndpointUrl#toUri also memoizes, so repeated reads no longer rebuild the URI. Guarded end to end in SyncClientHandlerTest: an interceptor reads the snapshot during modifyHttpRequest and asserts the URI has no query string, which fails if the snapshot is ever built from getUri() again.
The javadoc reference that used it was removed, and Checkstyle's UnusedImports does not process javadoc, so the import became a build error.
A customer unit test sets this attribute, so throwing on write is too strict. Writing now replaces the backing endpoint snapshot via EndpointUrl#fromUri, which also pre-populates the cached URI, so a written value reads back exactly as written including its query string.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Build testing only.
Motivation and Context
Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License