Web console: single instance of axios for all network requests #10408

AshishKapoor · 2020-09-18T13:44:50Z

Fixes #10402. As proposed.

Description

Make use of a single instance of axios for the network requests.

Makes it easy to configure network calls with axios

This PR has:

been self-reviewed.
been tested in a test Druid cluster.

Key changed/added classes in this PR

axios imports

Sincerely
Ashish Kapoor

This is in effect with the issue id: 10402

vogievetsky · 2020-09-18T17:47:11Z

Isn't the current usage of Axios already using a single (global) instance? As defined here: https://github.com/axios/axios/blob/master/lib/axios.js#L29

Is the idea of this to migrate the instance configuration from the defaults ( https://github.com/apache/druid/blob/master/web-console/src/entry.ts#L72 ) to this singleton Api file?

AshishKapoor · 2020-09-18T18:57:26Z

Is the idea of this to migrate the instance configuration from the defaults ( https://github.com/apache/druid/blob/master/web-console/src/entry.ts#L72 ) to this singleton Api file?

@vogievetsky Yes, you are correct!

One place to configure axios for the network requests and services. This way we can further define and separate all the network requests out of the views (in future).

vogievetsky

This PR currently breaks the existing configuration route.
Try adding "customHeaders": { 'x-test-header': 'this-is-a-test' } to the console-config.js file and see that the custom headers will not be applied. This is because while you are creating a custom instance you are still setting Api.defaults which will not be read now.

I suggest you make a method called Api.initialize that will take the configs and create the instance. It must be called before any API requests can go through (in entry.ts).

vogievetsky · 2020-09-20T19:25:43Z

web-console/src/entry.ts

@@ -70,14 +70,14 @@ if (typeof consoleConfig.title === 'string') {
 }

 if (consoleConfig.baseURL) {
-  axios.defaults.baseURL = consoleConfig.baseURL;
+  Api.defaults.baseURL = consoleConfig.baseURL;


These will now be ignored

AshishKapoor · 2020-09-21T22:04:15Z

*@vogievetsky* Fixed the issue and created an initialize method. Please let me know how we can make it better.

…

On Mon, Sep 21, 2020 at 12:59 AM Vadim Ogievetsky ***@***.***> wrote: ***@***.**** requested changes on this pull request. This PR currently breaks the existing configuration route. Try adding "customHeaders": { 'x-test-header': 'this-is-a-test' } to the console-config.js file and see that the custom headers will not be applied. This is because while you are creating a custom instance you are still setting Api.defaults which will not be read now. I suggest you make a method called Api.initialize that will take the configs and create the instance. It must be called before any API requests can go through (in entry.ts). ------------------------------ In web-console/src/entry.ts <#10408 (comment)>: > @@ -70,14 +70,14 @@ if (typeof consoleConfig.title === 'string') { } if (consoleConfig.baseURL) { - axios.defaults.baseURL = consoleConfig.baseURL; + Api.defaults.baseURL = consoleConfig.baseURL; These will now be ignored — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10408 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHWJIYP624GA2XUOSCN2TTSGZJYVANCNFSM4RSAOROQ> .

-- Sincerely Ashish Kapoor

ccaominh · 2020-09-21T22:09:30Z

This PR currently breaks the existing configuration route.
Try adding "customHeaders": { 'x-test-header': 'this-is-a-test' } to the console-config.js file and see that the custom headers will not be applied. This is because while you are creating a custom instance you are still setting Api.defaults which will not be read now.

I suggest you make a method called Api.initialize that will take the configs and create the instance. It must be called before any API requests can go through (in entry.ts).

Is it possible to add a test so that CI can help ensure this doesn't break in the future?

AshishKapoor · 2020-09-23T14:52:22Z

This PR currently breaks the existing configuration route.
Try adding "customHeaders": { 'x-test-header': 'this-is-a-test' } to the console-config.js file and see that the custom headers will not be applied. This is because while you are creating a custom instance you are still setting Api.defaults which will not be read now.
I suggest you make a method called Api.initialize that will take the configs and create the instance. It must be called before any API requests can go through (in entry.ts).

Is it possible to add a test so that CI can help ensure this doesn't break in the future?

@ccaominh maybe we can re-run the failed travis-cI test case. Please let me know how I can get it fixed.

ccaominh · 2020-09-23T18:13:22Z

This PR currently breaks the existing configuration route.
Try adding "customHeaders": { 'x-test-header': 'this-is-a-test' } to the console-config.js file and see that the custom headers will not be applied. This is because while you are creating a custom instance you are still setting Api.defaults which will not be read now.
I suggest you make a method called Api.initialize that will take the configs and create the instance. It must be called before any API requests can go through (in entry.ts).

Is it possible to add a test so that CI can help ensure this doesn't break in the future?

@ccaominh maybe we can re-run the failed travis-cI test case. Please let me know how I can get it fixed.

I've retriggered (Compile=openjdk8, Run=openjdk8) compaction integration test since the failure doesn't look related to this PR.

I think it'd be useful to add a web console test that ensures that the custom header, etc. are preserved.

vogievetsky · 2020-09-23T22:17:24Z

web-console/src/singletons/api.ts

@@ -18,4 +18,9 @@

 import axios from 'axios';

-export const Api = axios.create();
+function initialize() {


I meant that instead of setting axios.defaults (above) you can pass the instance cnfiguration to the initialize method. So the initialize method would take { baseURL, headers } etc as an argument.

AshishKapoor · 2020-10-10T08:43:52Z

web-console/src/singletons/api.ts

+export const API_ENDPOINTS = {
+  status: '/status',
+  properties: '/status/properties',
+  supervisor: '/druid/indexer/v1/supervisor',
+  supervisorFull: '/druid/indexer/v1/supervisor?full',
+  supervisorResumeAll: '/druid/indexer/v1/supervisor/resumeAll',
+  supervisorSuspendAll: '/druid/indexer/v1/supervisor/suspendAll',
+  supervisorTerminateAll: '/druid/indexer/v1/supervisor/terminateAll',
+  task: '/druid/indexer/v1/task',
+  tasks: '/druid/indexer/v1/tasks',
+  worker: '/druid/indexer/v1/worker',
+  workers: '/druid/indexer/v1/workers',
+  datasources: '/druid/coordinator/v1/datasources',
+  datasourcesSimple: '/druid/coordinator/v1/datasources?simple',
+  overlordStatus: '/proxy/overlord/status',
+  overlordStatusProperties: '/proxy/overlord/status/properties',
+  coordinatorStatus: '/proxy/coordinator/status',
+  coordinatorStatusProperties: '/proxy/coordinator/status/properties',
+  coordinatorCompactionCompact: '/druid/coordinator/v1/compaction/compact',
+  coordinatorRules: '/druid/coordinator/v1/rules',
+  coordinatorLookupsConfig: '/druid/coordinator/v1/lookups/config',
+  coordinatorLookupsConfigDiscover: '/druid/coordinator/v1/lookups/config?discover=true',
+  coordinatorLookupsConfigAll: '/druid/coordinator/v1/lookups/config/all',
+  coordinatorConfig: '/druid/coordinator/v1/config',
+  coordinatorConfigCompaction: '/druid/coordinator/v1/config/compaction',
+  coordinatorLoadStatusSimple: '/druid/coordinator/v1/loadstatus?simple',
+  coordinatorMetadataDatasources: '/druid/coordinator/v1/metadata/datasources',
+  coordinatorMetadataDatasourcesIncludeDisabled:
+    '/druid/coordinator/v1/metadata/datasources?includeDisabled',
+  coordinatorCompactionStatus: '/druid/coordinator/v1/compaction/status',
+  coordinatorTiers: '/druid/coordinator/v1/tiers',
+  coordinatorLookupsStatus: '/druid/coordinator/v1/lookups/status',
+  coordinatorServersSimple: '/druid/coordinator/v1/servers?simple',
+  coordinatorLoadqueueSimple: '/druid/coordinator/v1/loadqueue?simple',
+  druidV2: '/druid/v2',
+  druidV2Sql: '/druid/v2/sql',
+};


@vogievetsky we can add all the dynamic endpoints later.

vogievetsky · 2020-10-12T00:32:33Z

web-console/src/views/datasource-view/datasource-view.tsx

@@ -425,7 +425,7 @@ GROUP BY 1`;
    return (
      <AsyncActionDialog
        action={async () => {
-          const resp = await axios.delete(
+          const resp = await Api.delete(
            `/druid/coordinator/v1/datasources/${datasourceToMarkAsUnusedAllSegmentsIn}`,


In the theme of listing all the API endpoints in once file, I think this one could be extracted also for consistency. Maybe extract it as /druid/coordinator/v1/datasources/%s and then do a .replace('%s', datasourceToMarkAsUnusedAllSegmentsIn) in this function?

How about this?

`${API_ENDPOINTS.datasources}/${datasourceToMarkAsUnusedAllSegmentsIn}`,

Hey @vogievetsky

done for all the remaining ones.

lgtm-com · 2020-11-23T14:47:03Z

This pull request introduces 1 alert when merging db2e46b into 4537016 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

Web console: single instance of axios for all network requests

dc54668

This is in effect with the issue id: 10402

vogievetsky added the Area - Web Console label Sep 18, 2020

vogievetsky requested changes Sep 20, 2020

View reviewed changes

add initialise method for axios instance and fixed custom headers

2a37ce3

AshishKapoor requested a review from vogievetsky September 21, 2020 22:04

vogievetsky requested changes Sep 23, 2020

View reviewed changes

AshishKapoor and others added 6 commits September 25, 2020 20:57

Merge branch 'master' into axios-single-instance

62640a6

Merge branch 'master' into axios-single-instance

4057b22

api instead of axios

510a7c5

added easy api endpoints together

9bf8faf

some more replacement

4c29e42

coordinator config fix

ea7bda0

AshishKapoor commented Oct 10, 2020

View reviewed changes

precedence

2d99acc

vogievetsky reviewed Oct 12, 2020

View reviewed changes

AshishKapoor and others added 7 commits October 12, 2020 13:36

final bits of dynamic endpoints for consistency

8efef82

refactor supervisor

d118041

refactor supervisor

2bd2e10

refactor worker

b63207f

refactor task and tasks

c5e3dbd

refactor coordinator lookups config

e673328

Merge branch 'master' into axios-single-instance

db2e46b

AshishKapoor closed this Dec 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web console: single instance of axios for all network requests #10408

Web console: single instance of axios for all network requests #10408

AshishKapoor commented Sep 18, 2020 •

edited

vogievetsky commented Sep 18, 2020

AshishKapoor commented Sep 18, 2020 •

edited

vogievetsky left a comment

vogievetsky Sep 20, 2020

AshishKapoor commented Sep 21, 2020 via email

ccaominh commented Sep 21, 2020

AshishKapoor commented Sep 23, 2020

ccaominh commented Sep 23, 2020

vogievetsky Sep 23, 2020

AshishKapoor Oct 10, 2020

vogievetsky Oct 12, 2020

AshishKapoor Oct 12, 2020

AshishKapoor Oct 12, 2020 •

edited

lgtm-com bot commented Nov 23, 2020

Web console: single instance of axios for all network requests #10408

Web console: single instance of axios for all network requests #10408

Conversation

AshishKapoor commented Sep 18, 2020 • edited

Description

Makes it easy to configure network calls with axios

Key changed/added classes in this PR

vogievetsky commented Sep 18, 2020

AshishKapoor commented Sep 18, 2020 • edited

vogievetsky left a comment

Choose a reason for hiding this comment

vogievetsky Sep 20, 2020

Choose a reason for hiding this comment

AshishKapoor commented Sep 21, 2020 via email

ccaominh commented Sep 21, 2020

AshishKapoor commented Sep 23, 2020

ccaominh commented Sep 23, 2020

vogievetsky Sep 23, 2020

Choose a reason for hiding this comment

AshishKapoor Oct 10, 2020

Choose a reason for hiding this comment

vogievetsky Oct 12, 2020

Choose a reason for hiding this comment

AshishKapoor Oct 12, 2020

Choose a reason for hiding this comment

AshishKapoor Oct 12, 2020 • edited

Choose a reason for hiding this comment

lgtm-com bot commented Nov 23, 2020

AshishKapoor commented Sep 18, 2020 •

edited

AshishKapoor commented Sep 18, 2020 •

edited

AshishKapoor Oct 12, 2020 •

edited