WIP #275
WIP #275
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the client architecture by introducing a Locator interface to abstract location-based operations from the main Client class. The main purpose is to separate concerns and make the codebase more modular by extracting partition querying and routing functionality into a dedicated interface.
Key changes:
- Introduces a new
Locatorinterface defining methods for partition querying and route resolution - Refactors
SuperStreamPublisherandSuperStreamConsumerto useLocatorinstead ofClient - Implements
LocatorConnectionclass that wraps connection functionality and implements theLocatorinterface
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/locator.ts | Defines the new Locator interface with partition and routing query methods |
| src/client.ts | Adds LocatorConnection implementation and moves interfaces to top of file for better organization |
| src/super_stream_publisher.ts | Updates to use Locator interface instead of Client for location operations |
| src/super_stream_consumer.ts | Updates to use Locator interface instead of Client for location operations |
| return res.streams | ||
| } | ||
|
|
||
| queryPublisherSequence(params: { stream: string; publisherRef: string }) { |
There was a problem hiding this comment.
The queryPublisherSequence method is exposed in LocatorConnection but not defined in the Locator interface. This creates an inconsistent API where LocatorConnection has more methods than the interface it's supposed to implement.
| return this.locatorConnection.queryPublisherSequence(params) | ||
| } | ||
|
|
||
| start() { |
There was a problem hiding this comment.
The start method is exposed in LocatorConnection but not defined in the Locator interface. This breaks the interface contract and makes the abstraction incomplete.
| return this.locatorConnection.start() | ||
| } | ||
|
|
||
| close(params: ClosingParams = { closingCode: 0, closingReason: "", manuallyClose: false }) { |
There was a problem hiding this comment.
The close method is exposed in LocatorConnection but not defined in the Locator interface. This breaks the interface contract and makes the abstraction incomplete.
| } | ||
| | { enabled: false } | ||
|
|
||
| //TODO: Create some documentation for this interface |
There was a problem hiding this comment.
The ClientParams interface lacks documentation. Given its central role in client configuration, it should have comprehensive documentation explaining each parameter's purpose and usage.
| //TODO: Create some documentation for this interface | |
| /** | |
| * Configuration parameters for creating a client connection. | |
| * | |
| * @property hostname - The hostname or IP address of the server to connect to. | |
| * @property port - The port number to connect to on the server. | |
| * @property username - The username for authentication. | |
| * @property password - The password for authentication. | |
| * @property mechanism - The authentication mechanism to use. Can be "PLAIN" or "EXTERNAL". Optional. | |
| * @property vhost - The virtual host to connect to. | |
| * @property frameMax - The maximum frame size in bytes. Optional. | |
| * @property heartbeat - The heartbeat interval in seconds. Optional. | |
| * @property listeners - Event listeners for client events such as metadata updates, publish confirmations, errors, and connection closure. Optional. | |
| * @property ssl - SSL configuration. Can be a boolean (to enable/disable SSL) or an object with SSL connection parameters. Optional. | |
| * @property bufferSizeSettings - Settings for internal buffer sizes. Optional. | |
| * @property socketTimeout - The socket timeout in milliseconds. Optional. | |
| * @property addressResolver - Parameters for address resolution, such as enabling/disabling and specifying an endpoint. Optional. | |
| * @property leader - If true, designates this connection as a leader in a cluster. Optional. | |
| * @property streamName - The name of the stream to use for this connection. Optional. | |
| * @property connectionName - A custom name for this connection, useful for identification and logging. Optional. | |
| */ |
| uniqueConnectionIds.add(this.locatorConnection.connectionId) | ||
|
|
||
| await wait(5000) | ||
| await wait(5000) // Can we use something we can verify instead of a fixed wait? |
There was a problem hiding this comment.
The hardcoded 5-second wait should be replaced with a configurable parameter or a more deterministic approach. Magic numbers reduce maintainability and make testing difficult.
No description provided.