Skip to content

Conversation

@stevejgordon
Copy link
Collaborator

This allows consumers to provide builders on a per-request basis, which is required in ingest for streaming bulk response.

  • Fixes potential null ref exceptions when disposing of streams.
  • Refactors IRequestConfiguration and RequestConfigurationDescriptor into their own files.
  • Pins the SDK to 8.x until we're ready for 9.x.

This allows consumers to provide builders on a per-request basis,
required in ingest for streaming bulk response.

Fixes potential null ref exceptions when disposing streams.

This also pins the SDK to 8.x until we're ready for 9.x
@stevejgordon stevejgordon marked this pull request as draft November 14, 2024 06:47
@stevejgordon stevejgordon marked this pull request as ready for review November 14, 2024 07:12
Copy link
Member

@flobernd flobernd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,

One downside is that this will create a new RequestData each time because local is not null. Although I don't have a solution for this.

Can the per request also utilize a request specific response type so that it can inject a global builders collection where CanBuild() does the type check for the request specific response type?

e.g class StreamBulkResponse : BulkResponse {}

Either way this is good additional functionality.

@stevejgordon
Copy link
Collaborator Author

Yeah, the RequestData allocation is a pain and something I'd like to avoid. I'll merge this and then play with some options to handle that differently. I had a few ideas I wanted to prototype and then could sync on those.

@stevejgordon stevejgordon merged commit 7d3a633 into main Nov 14, 2024
5 checks passed
@stevejgordon stevejgordon deleted the per-req-response-builders branch November 14, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants