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

WaitFor for Qdrant #5767

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

WaitFor for Qdrant #5767

wants to merge 3 commits into from

Conversation

Alirexaa
Copy link
Contributor

@Alirexaa Alirexaa commented Sep 18, 2024

Description

Adds WaitFor support for Qdrant

Related: #5645

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 18, 2024
@davidfowl davidfowl requested review from mitchdenny and sebastienros and removed request for radical and sebastienros September 18, 2024 18:35
ConnectionString = connectionString
};

if (connectionBuilder.ContainsKey("Endpoint") && Uri.TryCreate(connectionBuilder["Endpoint"].ToString(), UriKind.Absolute, out var serviceUri))
Copy link
Member

Choose a reason for hiding this comment

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

I know this hasn't been done anywhere else but maybe we could start using TryGetValue to save a lookup. Aspire will be so much faster after that ;)

Suggested change
if (connectionBuilder.ContainsKey("Endpoint") && Uri.TryCreate(connectionBuilder["Endpoint"].ToString(), UriKind.Absolute, out var serviceUri))
if (connectionBuilder.TryGetValue("Endpoint", out var endpoint) && Uri.TryCreate(endpoint.ToString(), UriKind.Absolute, out var serviceUri))

var factory = sp.GetRequiredService<IHttpClientFactory>();
var client = factory.CreateClient();
client.BaseAddress = endpoint;
if (key is not null)
Copy link
Member

Choose a reason for hiding this comment

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

Is it technically possible that this client has any default headers coming from other configuration?

Suggested change
if (key is not null)
if (key is not null && !client.DefaultRequestHeaders.Contains("Api-Key"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used named clients, so we don't need this.

src/Aspire.Hosting.Qdrant/QdrantBuilderExtensions.cs Outdated Show resolved Hide resolved
@sebastienros
Copy link
Member

Digging a little more into the HttpClient usage here, I wonder if it would be better to resolve it in CheckHealthAsync instead and dispose it in this same method to follow a better IHttpClientFactory pattern instead of keeping a long-lived instance around. And then the named client could be configured on registration like this:

builder.Services.AddHttpClient(
    "qdrant-healthchecks",
    client =>
    {
        // Set the base address of the named client.
        client.BaseAddress = ...;

        client.DefaultRequestHeaders.Add("Api-Key", ...)
    });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants