-
Notifications
You must be signed in to change notification settings - Fork 132
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.
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
404 describe #3104
404 describe #3104
Changes from 4 commits
1eac49a
87030b6
06a42a3
71daff4
9bca4c5
3fca224
1fa56b6
e9588fc
35de799
2620dff
328f0a4
31b5d55
725f21f
d16d2ca
62a6084
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,8 +8,8 @@ import {updateStatus} from "src/js/flows/lake/update-status" | |
import Current from "src/js/state/Current" | ||
import LakeStatuses from "src/js/state/LakeStatuses" | ||
import styled from "styled-components" | ||
import {initCurrentTab} from "src/js/flows/initCurrentTab" | ||
import {invoke} from "src/core/invoke" | ||
import {Active} from "src/models/active" | ||
|
||
const SpinnerWrap = styled.div` | ||
width: 100%; | ||
|
@@ -27,6 +27,7 @@ export function InitLake({children}) { | |
useLayoutEffect(() => { | ||
if (status) return | ||
if (lake) dispatch(updateStatus(lake.id)) | ||
Active.lake.sync() | ||
}, [lake?.id, status]) | ||
|
||
useEffect(() => { | ||
|
@@ -35,7 +36,7 @@ export function InitLake({children}) { | |
|
||
switch (status) { | ||
case "disconnected": | ||
return <ConnectionError onRetry={() => dispatch(initCurrentTab())} /> | ||
return <ConnectionError onRetry={() => dispatch(updateStatus(lake.id))} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
All of this "lake" status/authentication code needs another look at some point. |
||
case "login-required": | ||
return <Login lake={lake} /> | ||
case "connected": | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,13 @@ import Current from "../state/Current" | |
import {Store} from "../state/types" | ||
import {defaultLake} from "./initLakeParams" | ||
import Window from "../state/Window" | ||
import {Lake} from "src/models/lake" | ||
|
||
export function initLake(store: Store) { | ||
const lakeId = Current.getLakeId(store.getState()) | ||
const id = lakeId || defaultLake().id | ||
// This sets up the lake's tabs | ||
const lake = Lake.find(id) | ||
lake.sync() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As soon as the app boots up, we sync the lake. |
||
store.dispatch(Window.setLakeId(id)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,11 +44,7 @@ export default async function initialize( | |
const api = new ZuiApi() | ||
const store = await initStore(api, renderer) | ||
const asyncTasks = initAsyncTasks(renderer) | ||
await initGlobals(store) | ||
await initLake(store) | ||
api.init(store.dispatch, store.getState) | ||
initDOM() | ||
initIpcListeners(store) | ||
initDomainModels({store}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the Domain Models need to be provisioned with the store variable before we call "initLake". So I re-ordered the steps here. |
||
initHandlers({ | ||
transition: startTransition, | ||
oldApi: api, | ||
|
@@ -59,9 +55,13 @@ export default async function initialize( | |
toast, | ||
asyncTasks, | ||
}) | ||
initDomainModels({ | ||
store, | ||
}) | ||
|
||
await initGlobals(store) | ||
await initLake(store) | ||
api.init(store.dispatch, store.getState) | ||
initDOM() | ||
initIpcListeners(store) | ||
|
||
ViewHandler.store = store | ||
setMenuContext({select: (fn) => fn(store.getState()), api}) | ||
initDebugGlobals(store, api) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,9 @@ export type LakeAttrs = { | |
version?: string | ||
authType: AuthType | ||
authData?: AuthData | ||
features?: { | ||
describe: boolean | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to add a "features" property the the data we cache about the lake. The only feature right now will be "describe". |
||
} | ||
|
||
export type AuthType = "none" | "auth0" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import {BrowserTab} from "./browser-tab" | |
import {Frame} from "./frame" | ||
import {getActiveTab} from "src/js/state/Tabs/selectors" | ||
import Editor from "src/js/state/Editor" | ||
import {Lake} from "./lake" | ||
|
||
export class Active extends DomainModel { | ||
static get tab() { | ||
|
@@ -36,4 +37,9 @@ export class Active extends DomainModel { | |
id: globalThis.windowId, | ||
}) | ||
} | ||
|
||
static get lake() { | ||
const id = this.select(Current.getLakeId) | ||
return Lake.find(id) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a shortcut to get the Active lake object. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,20 @@ | ||
import {DomainModel} from "src/core/domain-model" | ||
import {LakeAttrs} from "../js/state/Lakes/types" | ||
import {Client} from "@brimdata/zed-js" | ||
import Slice from "src/js/state/Lakes" | ||
import LakeStatuses from "src/js/state/LakeStatuses" | ||
import {LakeStatus} from "src/js/state/LakeStatuses/types" | ||
import {FeatureDetector} from "./lake/feature-detector" | ||
|
||
type LakeModelAttrs = LakeAttrs & {status: LakeStatus} | ||
|
||
export class Lake extends DomainModel<LakeModelAttrs> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we are adding the methods we need to the Lake model. I've added:
|
||
static find(id: string) { | ||
const attrs = this.select(Slice.id(id)) | ||
const status = this.select(LakeStatuses.get(id)) | ||
return new Lake({...attrs, status}) | ||
} | ||
|
||
export class Lake extends DomainModel<LakeAttrs> { | ||
getAddress(): string { | ||
return this.attrs.port | ||
? [this.attrs.host, this.attrs.port].join(":") | ||
|
@@ -39,4 +52,37 @@ export class Lake extends DomainModel<LakeAttrs> { | |
get version() { | ||
return this.attrs.version | ||
} | ||
|
||
get client() { | ||
return new Client(this.getAddress()) | ||
} | ||
|
||
get features() { | ||
return this.attrs.features ?? ({} as any) | ||
} | ||
|
||
async sync() { | ||
// Also authenticate (later) | ||
// Also check status (later) | ||
const detector = new FeatureDetector(this) | ||
const features = await detector.detect() | ||
this.update({features}) | ||
} | ||
|
||
update(attrs: Partial<LakeModelAttrs>) { | ||
this.attrs = {...this.attrs, ...attrs} | ||
this.save() | ||
} | ||
|
||
save() { | ||
const {status: _, ...attrs} = this.attrs | ||
this.dispatch(Slice.add(attrs)) | ||
} | ||
|
||
isRunning() { | ||
return this.client | ||
.version() | ||
.then(() => true) | ||
.catch(() => false) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import {Lake} from "../lake" | ||
|
||
export class FeatureDetector { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class is responsible for detecting all the features we want. |
||
constructor(public lake: Lake) {} | ||
|
||
async hasDescribe() { | ||
try { | ||
const resp = await this.lake.client.describeQuery("from :pools") | ||
return !resp.error | ||
} catch (e) { | ||
if (/404/.test(e.message)) return false | ||
else throw e | ||
} | ||
} | ||
|
||
async detect() { | ||
return { | ||
describe: await this.hasDescribe(), | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,11 @@ export class SessionPageHandler extends ViewHandler { | |
const program = this.select(Current.getQueryText) | ||
const history = this.select(Current.getHistory) | ||
|
||
if (!Active.lake.features.describe) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And finally, if the lake does not have describe, do the minimum required and return early. |
||
this.dispatch(QueryInfo.merge({isParsed: true})) | ||
return | ||
} | ||
|
||
fetchQueryInfo(program).then((info) => { | ||
this.dispatch(QueryInfo.set({isParsed: true, ...info})) | ||
const poolName = this.select(QueryInfo.getPoolName) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,22 +52,24 @@ export abstract class BaseClient { | |
|
||
async describeQuery( | ||
query: string, | ||
pool?: string, | ||
options: { signal?: AbortSignal; timeout?: number } = {} | ||
options: { signal?: AbortSignal; timeout?: number; pool?: string } = {} | ||
) { | ||
const head = pool ? { pool } : null; | ||
const head = options.pool ? { pool: options.pool } : null; | ||
const abortCtl = wrapAbort(options.signal); | ||
const result = await this.send({ | ||
const response = await this.request({ | ||
method: 'POST', | ||
path: `/query/describe`, | ||
body: JSON.stringify({ query, head }), | ||
contentType: 'application/json', | ||
format: 'json', | ||
signal: abortCtl.signal, | ||
timeout: options.timeout, | ||
dontRejectError: true, | ||
}); | ||
return result.json(); | ||
if (response.ok || response.status === 400) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re-arranged the code to allow for an error response from the server. If the describe endpoint returns code 400, there is a problem with the query, but we don't want to throw an error. We want to report the problem to the user. So we parse the JSON as an expected response. |
||
return response.json(); | ||
} else { | ||
throw createError(await parseContent(response)); | ||
} | ||
} | ||
|
||
async createPool(name: string, opts: Partial<Types.CreatePoolOpts> = {}) { | ||
|
@@ -145,18 +147,7 @@ export abstract class BaseClient { | |
${this.baseURL}/query`; | ||
} | ||
|
||
protected async send(opts: { | ||
method: 'GET' | 'POST' | 'DELETE' | 'PUT'; | ||
path: string; | ||
body?: Types.IsoBody; | ||
format?: Types.ResponseFormat; | ||
signal?: Types.IsoAbortSignal; | ||
headers?: Record<string, string>; | ||
timeout?: number; | ||
contentType?: string; | ||
duplex?: 'half'; | ||
dontRejectError?: boolean; | ||
}) { | ||
protected request(opts: Types.RequestOpts) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I separated the creation of the request into this request method. The existing The |
||
const abortCtl = wrapAbort(opts.signal); | ||
const clearTimer = this.setTimeout(() => { | ||
abortCtl.abort(); | ||
|
@@ -169,22 +160,21 @@ export abstract class BaseClient { | |
if (this.auth) { | ||
headers['Authorization'] = `Bearer ${this.auth}`; | ||
} | ||
|
||
const resp = await this.fetch(this.baseURL + opts.path, { | ||
const promise = this.fetch(this.baseURL + opts.path, { | ||
method: opts.method, | ||
headers: headers, | ||
// eslint-disable-next-line | ||
// @ts-ignore | ||
signal: abortCtl.signal, | ||
// eslint-disable-next-line | ||
// @ts-ignore | ||
body: opts.body, | ||
// eslint-disable-next-line | ||
// @ts-ignore | ||
duplex: opts.duplex, | ||
}); | ||
clearTimer(); | ||
if (resp.ok || opts.dontRejectError) { | ||
promise.finally(clearTimer); | ||
return promise; | ||
} | ||
|
||
protected async send(opts: Types.RequestOpts) { | ||
const resp = await this.request(opts); | ||
if (resp.ok) { | ||
return resp; | ||
} else { | ||
return Promise.reject(createError(await parseContent(resp))); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever the current lake id changes, or it's connection status changes, sync what info we have with the lake server.