ClickHouse / clickhouse-js

Official JS client for ClickHouse DB
https://clickhouse.com
Apache License 2.0
205 stars 25 forks source link

Drain insert HTTP response #203

Closed varrocs closed 11 months ago

varrocs commented 11 months ago

In case of an insert call drain the HTTP response instead of destroying it.

Summary

Calling destroy eventually closed the socket even if keep-alive was set. This was a performance limitation. https://github.com/ClickHouse/clickhouse-js/issues/202

Checklist

Delete items not relevant to your PR:

Note: I don't know how to test this

Note: I don't know if it applies to my change

CLAassistant commented 11 months ago

CLA assistant check
All committers have signed the CLA.

slvrtrn commented 11 months ago

I don't know how to test this

If we want to test it, we need to spy on sockets somehow (with max connections set to 1); I am afraid there is no other way.

(please don't mind failing Cloud tests for now; for some reason, Actions cannot fetch the secret properly for the community PRs).

varrocs commented 11 months ago

If we want to test it, we need to spy on sockets somehow

Well, let's say I don't insist on testing this.

varrocs commented 11 months ago

If we want to test it, we need to spy on sockets somehow

So it's me asking if you accept the pull request without writing tests to test if the consecutive requests are going on the same socket

slvrtrn commented 11 months ago

@varrocs, I think we can live without such a test for now cause nothing else is broken, and we have a specific test for consecutive inserts with one socket, IIRC (and they pass).

I'd only ask you to confirm that with this change, the TCP dump looks like the fixed version mentioned here: https://github.com/ClickHouse/clickhouse-js/issues/202#issue-1926461538

varrocs commented 11 months ago

Yes you have such test (with a comment that saying that how strange it is that the socket mostly reopened even with keep alive) And yes, the tcpdump looks good.