From 5b1fd598df1242bf30065d59d5f9807f865ad1d5 Mon Sep 17 00:00:00 2001 From: Mikhail Cheshkov Date: Sat, 23 Nov 2024 14:08:14 +0200 Subject: [PATCH 1/3] fix(clickhouse-driver): Remove SQL from ClickHouse error messages --- packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts b/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts index ddc7571b2f071..4de83da8cb1ed 100644 --- a/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts +++ b/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts @@ -260,7 +260,7 @@ export class ClickHouseDriver extends BaseDriver implements DriverInterface { return results; } catch (e) { // TODO replace string formatting with proper cause - throw new Error(`Query failed; cause: ${e}; query id: ${queryId}; SQL: ${query}`); + throw new Error(`Query failed: ${e}; query id: ${queryId}`); } }); } @@ -409,7 +409,7 @@ export class ClickHouseDriver extends BaseDriver implements DriverInterface { } catch (e) { await client.close(); // TODO replace string formatting with proper cause - throw new Error(`Stream query failed; cause: ${e}; query id: ${queryId}; SQL: ${query}`); + throw new Error(`Stream query failed: ${e}; query id: ${queryId}`); } } @@ -554,7 +554,7 @@ export class ClickHouseDriver extends BaseDriver implements DriverInterface { await this.command(createTableSql); } catch (e) { // TODO replace string formatting with proper cause - throw new Error(`Create table failed; cause: ${e}; SQL: ${createTableSql}`); + throw new Error(`Create table failed: ${e}`); } } From a97efbfc2d3f359a8fccf68f5e706bdffe2c5e6c Mon Sep 17 00:00:00 2001 From: Mikhail Cheshkov Date: Sat, 23 Nov 2024 14:56:32 +0200 Subject: [PATCH 2/3] fix(clickhouse-driver): Actually handle ping result --- packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts b/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts index 4de83da8cb1ed..bd5f5a3c55637 100644 --- a/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts +++ b/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts @@ -185,7 +185,11 @@ export class ClickHouseDriver extends BaseDriver implements DriverInterface { const { signal } = abortController; const promise = (async () => { - await this.client.ping(); + const pingResult = await this.client.ping(); + if (!pingResult.success) { + // TODO replace string formatting with proper cause + throw new Error(`Connection check failed: ${pingResult.error}`); + } signal.throwIfAborted(); // Queries sent by `fn` can hit a timeout error, would _not_ get killed, and continue running in ClickHouse // TODO should we kill those as well? From a2eedb44fca7f0549a76f801d273f9545d7ed347 Mon Sep 17 00:00:00 2001 From: Mikhail Cheshkov Date: Sat, 23 Nov 2024 14:57:47 +0200 Subject: [PATCH 3/3] fix(clickhouse-driver): Handle AggregateError in ping result --- packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts b/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts index bd5f5a3c55637..6878647941092 100644 --- a/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts +++ b/packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts @@ -188,7 +188,12 @@ export class ClickHouseDriver extends BaseDriver implements DriverInterface { const pingResult = await this.client.ping(); if (!pingResult.success) { // TODO replace string formatting with proper cause - throw new Error(`Connection check failed: ${pingResult.error}`); + // pingResult.error can be AggregateError when ClickHouse hostname resolves to multiple addresses + let errorMessage = pingResult.error.toString(); + if (pingResult.error instanceof AggregateError) { + errorMessage = `Aggregate error: ${pingResult.error.message}; errors: ${pingResult.error.errors.join('; ')}`; + } + throw new Error(`Connection check failed: ${errorMessage}`); } signal.throwIfAborted(); // Queries sent by `fn` can hit a timeout error, would _not_ get killed, and continue running in ClickHouse