feat(client): add ping method to http/s sender#45
Conversation
|
|
||
| // 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.
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.
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.
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.
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.
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.
OK, let's think a bit more. Maybe we can come up with a better idea.
There was a problem hiding this comment.
@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.
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
/pingendpoint 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