Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#sendRequest ought to accept request header #518

Closed
k-wall opened this issue Aug 4, 2023 · 3 comments · Fixed by #621
Closed

#sendRequest ought to accept request header #518

k-wall opened this issue Aug 4, 2023 · 3 comments · Fixed by #621
Assignees
Labels
enhancement New feature or request

Comments

@k-wall
Copy link
Contributor

k-wall commented Aug 4, 2023

Is your feature request related to a problem? Please describe.

As a filter author using the sendRequest API, I may need to specify a header. A likely user case is that I need to set clientId on the request, which currently I am unable to do. The implementation current manufactures an clientId.

There's also a separate argument for API consistency - the rest of the API already expects the header, body tuple.

Describe the solution you'd like

#sendRequest(@Nullable RequestHeaderData, @NonNull ApiMessage)
  • ApiMessage will be validated as being a class with the name RequestData otherwise throw IllegalArgumentException
  • If RequestHeaderData is null, the implementation will manufacture one. For apiVersion it should default to the highest supported version that was agreed during ApiVersion negotiation.
  • If RequestHeaderData is not null
    • the apiVersion should be clamped by the range agreed by ApiVersion negotiation, but otherwise the value respected. This gives the user a mechanism to influence the version. This idiom would emerge that indicates to Kroxylicious to use the highest version it can.
         sendRequest(new RequestHeaderData().setRequestApiVersion(ProduceRequestData.HIGHEST_SUPPORTED_VERSION), new ProduceRequestData())
    
    • the RequestHeaderData#apiKey will be overwritten by message.apiKey(). RequestHeaderData#apiKey defaults to 0 which is actually means PRODUCE.
    • other header fields, clientId, additionalTags would be respected.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

@k-wall k-wall added the enhancement New feature or request label Aug 4, 2023
@robobario
Copy link
Contributor

robobario commented Sep 6, 2023

We discussed in our call whether this should go into 0.3.0 along with all the other breaking changes. We decided it doesn't have to be in 0.3.0 and that we should start firming up our API stability, so this might be implemented as a second sendRequest method and we can start going through (quite rapid) deprecate+delete of the old method.

@robobario
Copy link
Contributor

robobario commented Sep 6, 2023

We should also make the response headers available, so instead of CompletionStage<ApiMessage> it should probably be CompletionStage<Response> where response wraps up header/body.

@k-wall
Copy link
Contributor Author

k-wall commented Sep 13, 2023

We should also make the response headers available, so instead of CompletionStage<ApiMessage> it should probably be CompletionStage<Response> where response wraps up header/body.

Agreed, or alternatively CompletionStage<ResponseHeader, ApiMessage>

@robobario robobario added this to the 0.3.0 milestone Sep 20, 2023
@SamBarker SamBarker removed this from the 0.3.0 milestone Sep 20, 2023
@k-wall k-wall self-assigned this Sep 20, 2023
@k-wall k-wall linked a pull request Sep 20, 2023 that will close this issue
7 tasks
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 21, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 22, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 22, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 22, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 22, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 22, 2023
k-wall added a commit that referenced this issue Sep 25, 2023
* Refactoring Filter sendRequest tests

* the out-of-band request future (created by the FilterHandler) now gets
  associated with the mimicked out-of-band response. 
* the FilterHarness now allows a channel to be built with multiple filters.
  this allows for better otherFiltersInChainCanFilterOutOfBand*().


Co-authored-by: Sam Barker <[email protected]>
Signed-off-by: Keith Wall <[email protected]>
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 25, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 25, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 26, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 26, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 27, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 27, 2023
@k-wall k-wall changed the title #sendRequest ought to accept header #sendRequest ought to accept request header and return the response header Sep 27, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 28, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 29, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Sep 29, 2023
@k-wall k-wall changed the title #sendRequest ought to accept request header and return the response header #sendRequest ought to accept request header ~~and return the response header~~ Sep 29, 2023
@k-wall k-wall changed the title #sendRequest ought to accept request header ~~and return the response header~~ #sendRequest ought to accept request header ~and return the response header~ Sep 29, 2023
@k-wall k-wall changed the title #sendRequest ought to accept request header ~and return the response header~ #sendRequest ought to accept request header Sep 29, 2023
k-wall added a commit to k-wall/kroxylicious that referenced this issue Oct 2, 2023
k-wall added a commit that referenced this issue Oct 2, 2023
* Fix #518: API - sendRequest ought to accept request header

* validate that the request api version falls within range supported by the Kafka implementation.
* remove deprecated #sendRequest

Signed-off-by: Keith Wall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants