Feature: Canner enterprise connector #172

onlyjackfrost · 2023-05-24T11:05:17Z

Description

What this PR does:

Support Canner Enterprise as a data source in VulcanSQL
Caching feature in VulcanSQL is also supported

We use PostgreSQL Wire Protocol to connect to canner enterprise and execute the sql in VulcanSQL.
Check the doc for more connecting info

The export flow:

Use Canner Enterprise API to create an async-query that will execute the sql statement and turn the query result into parquet files and stored in S3
wait the async-query to finished
Get the presigned URLs of this async-query and download.
For the canner API info, please check here

Issue ticket number

issue #169

Additional Context

The Canner async-query APIs have stored the query result as parquet files in S3, so we don't need to do it by ourselves.

vercel · 2023-05-24T11:05:20Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
vulcan-sql-document	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 5, 2023 10:29am

codecov-commenter · 2023-05-25T06:25:17Z

Codecov Report

Patch coverage: 84.74% and project coverage change: -0.22 ⚠️

Comparison is base (885720c) 91.13% compared to head (0b0de36) 90.91%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #172      +/-   ##
===========================================
- Coverage    91.13%   90.91%   -0.22%     
===========================================
  Files          314      321       +7     
  Lines         5006     5183     +177     
  Branches       668      695      +27     
===========================================
+ Hits          4562     4712     +150     
- Misses         316      337      +21     
- Partials       128      134       +6

Flag	Coverage Δ
extension-driver-canner	`84.74% <84.74%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s/extension-driver-canner/src/lib/cannerAdapter.ts	`69.04% <69.04%> (ø)`
...kages/extension-driver-canner/test/cannerServer.ts	`80.00% <80.00%> (ø)`
...xtension-driver-canner/src/lib/cannerDataSource.ts	`85.05% <85.05%> (ø)`
packages/extension-driver-canner/src/index.ts	`100.00% <100.00%> (ø)`
packages/extension-driver-canner/src/lib/config.ts	`100.00% <100.00%> (ø)`
...ages/extension-driver-canner/src/lib/sqlBuilder.ts	`100.00% <100.00%> (ø)`
...ages/extension-driver-canner/src/lib/typeMapper.ts	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

kokokuo

Besides some typo, comment and handle error suggestions, others LGTM 👍 👍

kokokuo · 2023-05-26T08:22:58Z

packages/extension-driver-canner/src/lib/cannerDataSource.ts

+    for (const profile of profiles) {
+      // try to connect by pg wire protocol and make request to api server
+      this.logger.debug(
+        `Initializing profile: ${profile.name} using pg driver`


How about renaming the pg driver to canner driver or canner driver by pg wire-protocol to recognize it when log?

kokokuo · 2023-05-30T01:45:45Z

packages/extension-driver-canner/src/lib/config.ts

@@ -0,0 +1,13 @@
+export interface IEnvConfig {
+  // indicates whether the extension is running in k8s


Typo: running on k8s, not running in k8s.

The environment variable IS_INTERNAL is true or false may not be clear to know running on k8s or not, because the env does not talk about k8s, so we may need to make our comment describe more ( or you could rename the variable if you don't mind ), otherwise, our member may not know why we need the env to indicate running on k8s especially, so we should also expose more message, like below:

When it's true means the extension is running on k8s, and the canner data source will send requests by HTTP protocol to make the request successful under the k8s cluster.

I'll rename this variable to "isOnKubernetes"

I suggest still adding the comment to your environment, to help members or other users know why we need to use the env and why set the value is true, we will use the HTTP protocol.

delivered in 8ed2dfd

kokokuo · 2023-05-30T01:54:38Z

packages/extension-driver-canner/src/lib/cannerAdapter.ts

+    this.ssl = ssl;
+  }
+
+  public async createAsyncQueryResultUrls(sql: string): Promise<string[]> {


How about adding the comment on the createAsyncQueryResultUrls method to describe what the methods could do?
Our partner or new member may not have the context to know why we need to get the result URLs. so may suggest you describe what the result urls could do :)

I'll add some comment to explain it, thanks.

kokokuo · 2023-05-30T03:27:47Z

packages/extension-driver-canner/src/lib/cannerAdapter.ts

+    });
+    const { restfulApiBaseEndpoint } = response.data;
+    if (!restfulApiBaseEndpoint) {
+      throw new Error(`restfulApiBaseEndpoint is not found`);


The restfulApiBaseEndpoint is a certain field from the /cluster-info api data, however, not all members know the fields of the /cluster-info API except they open the canner enterprise to dig in.

Suggest using a more usual or common description to show, e.g:

The restful API base endpoint is not found, please check "restfulApiBaseEndpoint" field from "/cluster-info" endpoint of Canner Enterprise

kokokuo · 2023-05-30T03:34:33Z

packages/extension-driver-canner/src/lib/cannerAdapter.ts

+      response = await this.workspaceRequest(
+        'get',
+        `/v2/async-queries/${requestId}`
+      );


Great to handle the status, but seems that you use twice to same code to call the requestId and get the current response status, maybe we could use a function expression under the method to reuse ( because the behavior is small, it's ok ):

const getResponse = async (requestId: string) => await this.workspaceRequest('get',`/v2/async-queries/${requestId}`) let response = await getResponse(requestId) .... while (!['FINISHED', 'FAILED'].includes(status)) { await new Promise((resolve) => setTimeout(resolve, 1000)); response = await getResponse(requestId); }

kokokuo · 2023-05-30T03:51:37Z

packages/extension-driver-canner/src/lib/cannerAdapter.ts

+      'get',
+      `/v2/async-queries/${requestId}/result/urls`
+    );
+    return data.urls || [];


Seems the /v2/async-queries/<requestId>/result/urls will throw an error from Canner Enterprise and generate an error message, maybe we should check if exist any errors and throw it.

Tips: we could handle the error in the WorkspaceRequest directly, then you don't need to handle it in different places

I'll use status to check and handle the error message from the canner server

kokokuo · 2023-05-30T04:01:53Z

packages/extension-driver-canner/src/lib/cannerAdapter.ts

+    this.ssl = ssl;
+  }
+
+  public async createAsyncQueryResultUrls(sql: string): Promise<string[]> {


Suggest using the logger.debug ( from our getLogger put in the core/utils ) to record at least steps:

send the async query to the Canner Enterprise

get the query result parquet files from URLs.

It could help the developer and QA to know the current status, working phase, and approximate location on the K8S when an error happened and the error is not caused by what we defined especially.

Do you miss the suggestion or ?

delivered in 8ed2dfd

kokokuo · 2023-05-30T04:03:48Z

packages/extension-driver-canner/test/cannerServer.ts

+      type: 'canner',
+      connection: {
+        host: process.env['CANNER_HOST'],
+        port: process.env['CANNER_PORT'] || 7432,


Just curious, why the default is 7432, 7432 port is PG wire-protocol default port ?

According to this introduction, the port of the PG wire protocol is 7432, so I set the default port to 7432

kokokuo · 2023-05-30T04:14:30Z

packages/extension-driver-canner/src/lib/cannerDataSource.ts

+        const response = await axios.get(url, {
+          responseType: 'stream',
+        });
+        const fileName = url.split('/').pop()?.split('?')[0] || `part${index}`;


Not sure why you split / and pop last slice, then split(?) and get the first value, because we may not know the url` pattern, could you comment to give a sample and explain to make others know?

I didn't find the pattern of a presigned-url from AWS, it's based on my observation and this StackOverflow.
The file name will be a substring that is after the last "/" and followed by the "?" and the query string

kokokuo · 2023-05-30T04:15:37Z

packages/extension-driver-canner/src/lib/cannerAdapter.ts

+    return urls;
+  }
+
+  private async workspaceRequest(


Great to define a method to reuse for calling the workspace request 👍

- can access data sources from canner through this extension - support cache feature in data source "canner enterprise"

kokokuo

Thanks for fixing the comment, here is still 4 suggestions ( including missed and incorrect handle solution, and comment suggestions.), Others LGTM 👍

kokokuo · 2023-06-05T01:36:22Z

packages/extension-driver-canner/src/lib/cannerAdapter.ts

+    this.ssl = ssl;
+  }
+
+  public async createAsyncQueryResultUrls(sql: string): Promise<string[]> {


Do you miss the suggestion or ?

kokokuo · 2023-06-05T02:01:05Z

packages/extension-driver-canner/src/lib/cannerAdapter.ts

+    if (response.status !== 200) {
+      throw new Error(
+        `Failed to get workspace request "${urlPath}" data, status: ${
+          response.status
+        }, data: ${JSON.stringify(response.data)}`
+      );
+    }


Do you survey the error handling of the axios ?

The axios will throw erro when the status is not 200, so according to your logistic, it won' t trigger the if-else, please:
https://www.axios-http.cn/docs/handling_errors
https://stackoverflow.com/questions/49967779/axios-handling-errors

Otherwise, you should also check the Canner Enterprise hot to respond the error and it code, please see how we handle the different errors and wrap to HTTP status code to 500 when happened the errors, the reason you could see the comment https://github.com/Canner/canner-ui/blob/5ea2e698ebad8ed953daabe362e773461701bd09/server/errors/formatUnifiedError.ts#L61.

Btw, we also use the axios in Canner to handle the restful API from SQL Engine, and they also will throw different error codes, so that why we use the try-catch to handle it: https://github.com/Canner/canner-ui/blob/5ea2e698ebad8ed953daabe362e773461701bd09/server/adapters/externalQueryTable.ts#L45

And calling the async-query API, it also may throw the domain error and cause the 500 status code.

delivered in 8ed2dfd

kokokuo · 2023-06-05T03:30:22Z

packages/extension-driver-canner/test/cannerDataSource.spec.ts

+it('Data source should throw when fail to export data', async () => {
+  // Arrange
+  sinon.default
+    .stub(CannerAdapter.prototype, 'createAsyncQueryResultUrls')


Suggest adding the comment to describe what reason so need to use prototype to stub the method, not usual solution :)

delivered in 8ed2dfd

kokokuo · 2023-06-05T03:32:34Z

packages/extension-driver-canner/src/lib/config.ts

@@ -0,0 +1,13 @@
+export interface IEnvConfig {
+  // indicates whether the extension is running in k8s


I suggest still adding the comment to your environment, to help members or other users know why we need to use the env and why set the value is true, we will use the HTTP protocol.

kokokuo

Thanks for fixing, LGTM 👍

onlyjackfrost changed the title ~~[DRAFT] Feature/canner enterprise connector~~ [DRAFT] Feature: Canner enterprise connector May 24, 2023

onlyjackfrost force-pushed the feature/canner-enterprise-connector branch from e51a8fa to 81e75e2 Compare May 25, 2023 06:21

vercel bot deployed to Preview May 25, 2023 06:22 View deployment

vercel bot deployed to Preview May 25, 2023 06:51 View deployment

vercel bot deployed to Preview May 25, 2023 07:38 View deployment

vercel bot deployed to Preview May 25, 2023 07:58 View deployment

onlyjackfrost force-pushed the feature/canner-enterprise-connector branch from ac69cbb to 2b06d37 Compare May 25, 2023 08:08

vercel bot deployed to Preview May 25, 2023 08:08 View deployment

onlyjackfrost changed the title ~~[DRAFT] Feature: Canner enterprise connector~~ Feature: Canner enterprise connector May 25, 2023

vercel bot deployed to Preview May 29, 2023 08:13 View deployment

kokokuo reviewed May 30, 2023

View reviewed changes

feat(extension-driver-canner): add canner enterprise connector

a65e56c

- can access data sources from canner through this extension - support cache feature in data source "canner enterprise"

onlyjackfrost force-pushed the feature/canner-enterprise-connector branch from 0b0de36 to a65e56c Compare June 1, 2023 03:14

vercel bot deployed to Preview June 1, 2023 03:14 View deployment

kokokuo linked an issue Jun 1, 2023 that may be closed by this pull request

Support Canner Enterprise DataSource #169

Closed

kokokuo reviewed Jun 5, 2023

View reviewed changes

chore: fix axios error handling, add comments, add logger for debugging

8ed2dfd

vercel bot deployed to Preview June 5, 2023 08:21 View deployment

chore: simplify error message

a646f5c

vercel bot deployed to Preview June 5, 2023 10:29 View deployment

kokokuo approved these changes Jun 6, 2023

View reviewed changes

kokokuo merged commit a0711f0 into develop Jun 6, 2023

kokokuo deleted the feature/canner-enterprise-connector branch June 6, 2023 11:16

onlyjackfrost mentioned this pull request Jun 7, 2023

Feature: Canner PAT authenticator #181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Canner enterprise connector #172

Feature: Canner enterprise connector #172

onlyjackfrost commented May 24, 2023

vercel bot commented May 24, 2023 •

edited

Loading

codecov-commenter commented May 25, 2023 •

edited

Loading

kokokuo left a comment

kokokuo May 26, 2023

kokokuo May 30, 2023

onlyjackfrost May 31, 2023

kokokuo Jun 5, 2023

onlyjackfrost Jun 5, 2023

kokokuo May 30, 2023

onlyjackfrost May 31, 2023

kokokuo May 30, 2023

kokokuo May 30, 2023 •

edited

Loading

kokokuo May 30, 2023

onlyjackfrost May 31, 2023

kokokuo May 30, 2023 •

edited

Loading

kokokuo Jun 5, 2023

onlyjackfrost Jun 5, 2023

kokokuo May 30, 2023

onlyjackfrost May 31, 2023

kokokuo May 30, 2023 •

edited

Loading

onlyjackfrost May 31, 2023

kokokuo May 30, 2023

kokokuo left a comment

kokokuo Jun 5, 2023

kokokuo Jun 5, 2023

onlyjackfrost Jun 5, 2023

kokokuo Jun 5, 2023

onlyjackfrost Jun 5, 2023

kokokuo Jun 5, 2023

kokokuo left a comment

		@@ -0,0 +1,13 @@
		export interface IEnvConfig {
		// indicates whether the extension is running in k8s

Feature: Canner enterprise connector #172

Feature: Canner enterprise connector #172

Conversation

onlyjackfrost commented May 24, 2023

Description

Issue ticket number

Additional Context

vercel bot commented May 24, 2023 • edited Loading

codecov-commenter commented May 25, 2023 • edited Loading

Codecov Report

kokokuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo left a comment

Choose a reason for hiding this comment

vercel bot commented May 24, 2023 •

edited

Loading

codecov-commenter commented May 25, 2023 •

edited

Loading

kokokuo May 30, 2023 •

edited

Loading

kokokuo May 30, 2023 •

edited

Loading

kokokuo May 30, 2023 •

edited

Loading