-
Notifications
You must be signed in to change notification settings - Fork 1
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
Rewrite to allow easier error handeling #3
base: main
Are you sure you want to change the base?
Rewrite to allow easier error handeling #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer: I'm not part of the Quickwit team, just an internet rando.
This is a great start @LessThanGreaterThan ! Left a few minor comments and will provide more feedback once I've been able to use this client for local prototyping.
return nil | ||
}), | ||
endpoint: endpoint, | ||
client: &http.Client{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is often desirable to make the underlying http client configurable. What do you think about exposing both the endpoint
and the client
, so that they can be set/overridden externally?
type QuickwitClient struct {
Endpoint string
Client *http.Client
}
The current
func NewQuickwitClient(endpoint string) *QuickwitClient{
…
}
could stay as-is for now until more options are needed.
} | ||
return nil, &errmsg | ||
} | ||
return &SearchResponse{}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that the quickwit response is never returned.
A small change like the following should get us a functional client! (tests needed of course)
diff --git a/client.go b/client.go
index 28b02c8..8da87ec 100644
--- a/client.go
+++ b/client.go
@@ -142,6 +142,7 @@ func (c *QuickwitClient) Search(indexId string, searchRequest SearchRequest) (*S
return nil, err
}
defer post.Body.Close()
+
if post.StatusCode != http.StatusOK {
msg, err := io.ReadAll(post.Body)
if err != nil {
@@ -154,7 +155,9 @@ func (c *QuickwitClient) Search(indexId string, searchRequest SearchRequest) (*S
}
return nil, &errmsg
}
- return &SearchResponse{}, nil
+ var searchResponse SearchResponse
+ err = json.NewDecoder(post.Body).Decode(&searchResponse)
+ return &searchResponse, err
}
There's probably a way to reduce some amount of duplication across all methods but that doesn't have to be done today.
I have rewritten parts of client to remove the
req
dependency as it makes error handling rather untransparent (which is also mentioned in #1 ) and switched to the inbuildnet/http
client.I've also added support for following apis:
creating index
deleting index
ingesting data
this is still a WIP, looking for feedback if this is something to consider, and if so, what parts still need polishing.