chore(nodejs): extract transport and buffer from sender, add back support for core http #48
Conversation
glasstiger
changed the title
| private readonly log: Logger; | ||
|
|
||
| /** | ||
| * Creates an instance of Sender. |
There was a problem hiding this comment.
| * Creates an instance of Sender. | |
| * Creates an instance of SenderBuffer. |
| timeoutMillis: number, | ||
| ): http.RequestOptions | https.RequestOptions { | ||
| return { | ||
| //protocol: this.secure ? "https:" : "http:", |
There was a problem hiding this comment.
A leftover? Shouldn't we specify the protocol?
| /** @classdesc | ||
| * The QuestDB client's API provides methods to connect to the database, ingest data, and close the connection. | ||
| * The supported protocols are HTTP and TCP. HTTP is preferred as it provides feedback in the HTTP response. <br> | ||
| * of <i>undici.Agent</i>. The Sender's own agent uses persistent connections with 1 minute idle timeout, pipelines requests default to 1. |
There was a problem hiding this comment.
nit: of undici.Agent. doesn't seem to be a complete sentence. Also, all of the transport and buffer jsdocs seem to be the same (The QuestDB client's API provides methods to connect...), so they don't seem to be very helpful.
| private readonly dispatcher: RetryAgent; | ||
|
|
||
| /** | ||
| * Creates an instance of Sender. |
There was a problem hiding this comment.
| * Creates an instance of Sender. | |
| * Creates an instance of UndiciTransport. |
| if (body.byteLength > 0) { | ||
| this.log( | ||
| "warn", | ||
| `Unexpected message from server: ${Buffer.from(body).toString()}`, |
There was a problem hiding this comment.
nit: the response may be large, so it's better to truncate it when logging.
| import { TcpTransport } from "./tcp"; | ||
| import { HttpTransport } from "./http/legacy"; | ||
|
|
||
| interface SenderTransport { |
There was a problem hiding this comment.
nit: shouldn't we have some jsdoc for the common interfaces like this one?
| private readonly jwk: Record<string, string>; | ||
|
|
||
| /** | ||
| * Creates an instance of Sender. |
There was a problem hiding this comment.
| * Creates an instance of Sender. | |
| * Creates an instance of TcpTransport. |
|
|
||
| /** | ||
| * Closes the TCP connection to the database. <br> | ||
| * Data sitting in the Sender's buffer will be lost unless flush() is called before close(). |
There was a problem hiding this comment.
nit: might be a good idea to log a warning when some data is left in the buffer when close() is called.
|
|
||
| /** @private */ socket; | ||
| private buffer: SenderBuffer; |
There was a problem hiding this comment.
nit: could be readonly?
| `Column value must be of type ${valueType}, received ${typeof value}`, | ||
| ); | ||
| this.log("debug", `Pending row count: ${this.pendingRowCount}`); | ||
| await this.tryFlush(); |
There was a problem hiding this comment.
In Java Sender auto_flush is only supported for HTTP transport, but this change enables it for TCP transport. Is that on purpose?
| log?: Logger; | ||
| agent?: Agent | http.Agent | https.Agent; | ||
|
|
||
| legacy_http?: boolean; |
There was a problem hiding this comment.
This option doesn't seem to be documented. Also, I wouldn't call the default HTTP client "legacy", so something like stdlib_http or default_http may be a better name.
Extracting the transport layer from
Sender, and introducing theSenderTransportinterface with different implementations:httpmodule with ILP over HTTPundicimodule with ILP over HTTPAlso extracting the buffer from
Senderinto a classSenderBuffer.Now the
Senderis mostly just wiring between a transport and a buffer, and responsible only for auto flushing.In fact, you could create your own transport and buffer, and use them without a
Senderif you are calling flush() manually.You could also create your own pools of buffers and transports, the transports could be even different types, and just flush your buffers when needed using one of the transports from the pool.
Also refactored and improved tests.
Introduced more types, such as Logger, ExtraOptions, TimestampUnit and MockConfig.
Fixed
anyandunknowntype warnings.