Skip to content

fix(clickhouse-driver): Tune ClickHouse errors: remove SQL, catch ping failure #8983

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 26, 2024
Merged

fix(clickhouse-driver): Tune ClickHouse errors: remove SQL, catch ping failure #8983

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b1fd59
fix(clickhouse-driver): Remove SQL from ClickHouse error messages
mcheshkov Nov 23, 2024
a97efbf
fix(clickhouse-driver): Actually handle ping result
mcheshkov Nov 23, 2024
a2eedb4
fix(clickhouse-driver): Handle AggregateError in ping result
mcheshkov Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions packages/cubejs-clickhouse-driver/src/ClickHouseDriver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,16 @@ 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
// 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
// TODO should we kill those as well?
Expand Down Expand Up @@ -260,7 +269,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}`);
}
});
}
Expand Down Expand Up @@ -409,7 +418,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}`);
}
}

Expand Down Expand Up @@ -554,7 +563,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}`);
}
}

Expand Down
Loading