-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat(client): add ping method to http/s sender #45
base: main
Are you sure you want to change the base?
Conversation
http_sender.go
Outdated
@@ -337,23 +340,56 @@ func (s *httpLineSender) At(ctx context.Context, ts time.Time) error { | |||
return nil | |||
} | |||
|
|||
// makeRequest returns a boolean if we need to retry the request | |||
func (s *httpLineSender) makeRequest(ctx context.Context) (bool, error) { | |||
func (s *httpLineSender) Ping(ctx context.Context) error { |
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.
Maybe we should execute a ping when we initialize an HTTP sender? Then we won't need a dedicated method.
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.
Yep, that seems reasonable and keeps the interface clean.
I would need to test how well this integrates into RedPanda Connect but I suspect that it's just a matter of where we return an error on failed ping.
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.
This is getting a bit hairy now when running integration tests. I've noticed the behavior on startup where QuestDB responds at /
with 200, but the /ping
endpoint still 404s (probably because the database isn't fully initialized yet).
In my older tests (with the explicit .Ping()
), I used an eventually assertion when testing the Ping()
function to account for this.
But now with ping baked into the constructor, we don't have that option. Unless we retry ping in the sender constructor? That seems like an anti-pattern to me, but maybe it's worth creating an option for? But then that option would be at the interface level so we would need to invalidate it for tcp senders, which is ugly, and part of the reason why we're baking it into the http sender's constructor in the first place... wdyt?
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.
I guess we could check for retriable errors and retry for retry_timeout
, just like we do it in Flush
. WDYT?
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.
I don't love it because 404 is actually an important error, and could be the result of user misconfiguration of a reverse proxy, for example. I'll need a bit of time to think about this.
In the meantime, ping
might not even be necessary for RedPanda Connect compatibility if we decide to go with a sender pool implementation. Using this pool, senders will be created and released during a WriteBatch
call instead of cached during the Connect
phase (see https://pkg.go.dev/github.com/benthosdev/benthos/v4@v4.27.0/public/service#example-package-OutputBatchedPlugin and https://pkg.go.dev/github.com/benthosdev/benthos/v4@v4.27.0/public/service#example-package-OutputBatchedPlugin for a bit more detail)
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.
OK, let's think a bit more. Maybe we can come up with a better idea.
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.
@puzpuzpuz I think it's time that we revisit this discussion. When running in Redpanda Connect, our client can emit some cryptic messages when we fail to successfully flush, making it difficult to diagnose and fix the underlying issue. I've encountered this when (mis)configuring TLS, and can imagine other users having problems, especially when setting this up for the first time.
I tried writing some code in our connector, but it didn't feel like the right place because we already have the following information stored in the client:
- QuestDB address
- Authentication (username/password or token)
- TLS settings
Maybe we incorporate this into the LineSenderPool
by adding a Ping()
method. This would keep the feature http-only AND we would not be modifying the LineSender
interface. It's cleaner for me (at least in this case) to have a separate method so I can call it in the connector's Connect method.
wdyt?
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.
How about sending the ping when you create a new pooled sender? Ping
method available on the pool would look weird IMO. It should be available on the sender itself, but indeed this doesn't play nicely with our common Sender
interface.
An http
/ping
endpoint is implemented in the database, but not included in this client. Ping can be useful for testing connection strings before attempting to send data over the wire