Skip to content

Commit

Permalink
more wip on batching
Browse files Browse the repository at this point in the history
  • Loading branch information
tgriesser committed Jun 23, 2022
1 parent 3cab26c commit 2517f8b
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 25 deletions.
2 changes: 1 addition & 1 deletion packages/app/cypress/e2e/specs_list_latest_runs.cy.ts
Expand Up @@ -149,7 +149,7 @@ describe('ACI - Latest runs and Average duration', { viewportWidth: 1200, viewpo
beforeEach(() => {
cy.loginUser()

cy.remoteGraphQLIntercept(async (obj) => {
cy.remoteGraphQLInterceptBatched(async (obj) => {
if (obj.result.data && 'cloudSpecByPath' in obj.result.data) {
obj.result.data.cloudSpecByPath = {
__typename: 'CloudProjectSpecNotFound',
Expand Down
2 changes: 1 addition & 1 deletion packages/app/src/specs/SpecsList.vue
Expand Up @@ -436,7 +436,7 @@ const collapsible = computed(() => {
})
const treeSpecList = computed(() => collapsible.value.tree.filter(((item) => !item.hidden.value)))

const { containerProps, list, wrapperProps, scrollTo } = useVirtualList(treeSpecList, { itemHeight: 40, overscan: 10 })
const { containerProps, list, wrapperProps, scrollTo } = useVirtualList(treeSpecList, { itemHeight: 40, overscan: 2 })

const scrollbarOffset = ref(0)

Expand Down
72 changes: 50 additions & 22 deletions packages/data-context/src/sources/CloudDataSource.ts
Expand Up @@ -9,7 +9,7 @@ import crypto from 'crypto'

import type { DataContext } from '..'
import getenv from 'getenv'
import { print, DocumentNode, ExecutionResult, GraphQLResolveInfo, OperationTypeNode } from 'graphql'
import { print, DocumentNode, ExecutionResult, GraphQLResolveInfo, OperationTypeNode, visit, OperationDefinitionNode } from 'graphql'
import {
createClient,
dedupExchange,
Expand Down Expand Up @@ -50,6 +50,7 @@ export interface CloudExecuteQuery {
}

export interface CloudExecuteRemote extends CloudExecuteQuery {
shouldBatch?: boolean
operationType?: OperationTypeNode
requestPolicy?: RequestPolicy
onUpdatedResult?: (data: any) => any
Expand Down Expand Up @@ -81,14 +82,9 @@ export class CloudDataSource {

constructor (private params: CloudDataSourceParams) {
this.#cloudUrqlClient = this.reset()
this.#batchExecutor = createBatchingExecutor(({ document, variables }) => {
debug(`Executing remote dashboard request %s, %j`, print(document), variables)

return this.#cloudUrqlClient.query(document, variables ?? {}, {
...this.#additionalHeaders,
requestPolicy: 'network-only',
}).toPromise()
})
this.#batchExecutor = createBatchingExecutor((config) => {
return this.#executeQuery(namedExecutionDocument(config.document), config.variables)
}, { maxBatchSize: 20 })

this.#batchExecutorBatcher = this.#makeBatchExecutionBatcher()
}
Expand Down Expand Up @@ -213,7 +209,11 @@ export class CloudDataSource {
return loading
}

loading = this.#batchExecutorBatcher.load(config).then(this.#formatWithErrors)
const query = config.shouldBatch
? this.#batchExecutorBatcher.load(config)
: this.#executeQuery(config.operationDoc, config.operationVariables)

loading = query.then(this.#formatWithErrors)
.then((op) => {
this.#pendingPromises.delete(stableKey)

Expand All @@ -235,6 +235,12 @@ export class CloudDataSource {
return loading
}

#executeQuery (operationDoc: DocumentNode, operationVariables: object = {}) {
debug(`Executing remote dashboard request %s, %j`, print(operationDoc), operationVariables)

return this.#cloudUrqlClient.query(operationDoc, operationVariables, { requestPolicy: 'network-only' }).toPromise()
}

isResolving (config: CloudExecuteQuery) {
const stableKey = this.#hashRemoteRequest(config)

Expand Down Expand Up @@ -330,18 +336,6 @@ export class CloudDataSource {
*/
#makeBatchExecutionBatcher () {
return new DataLoader<CloudExecuteRemote, any>(async (toBatch) => {
const first = toBatch[0]

// If we only have a single entry, we can just hit the query directly,
// without rewriting anything - this makes the queries simpler in most cases in the app
if (toBatch.length === 1 && first) {
return [this.#cloudUrqlClient.query(first.operation, first.operationVariables ?? {}, {
...this.#additionalHeaders,
requestPolicy: 'network-only',
}).toPromise()]
}

// Otherwise run this through batchExecutor:
return Promise.allSettled(toBatch.map((b) => {
return this.#batchExecutor({
operationType: 'query',
Expand All @@ -358,3 +352,37 @@ export class CloudDataSource {
return val instanceof Error ? val : new Error(val)
}
}

/**
* Adds "batchExecutionQuery" to the query that we generate from the batch loader,
* useful to key off of in the tests.
*/
function namedExecutionDocument (document: DocumentNode) {
let hasReplaced = false

return visit(document, {
enter () {
if (hasReplaced) {
return false
}

return
},
OperationDefinition (op) {
if (op.name) {
return op
}

hasReplaced = true
const namedOperationNode: OperationDefinitionNode = {
...op,
name: {
kind: 'Name',
value: 'batchTestRunnerExecutionQuery',
},
}

return namedOperationNode
},
})
}
4 changes: 4 additions & 0 deletions packages/data-context/src/sources/RemoteRequestDataSource.ts
Expand Up @@ -19,6 +19,7 @@ interface MaybeLoadRemoteFetchable extends CloudExecuteQuery {
shouldFetch?: boolean
remoteQueryField: string
isMutation: boolean
shouldBatch?: boolean
}

interface OperationDefinition {
Expand Down Expand Up @@ -65,6 +66,7 @@ export class RemoteRequestDataSource {
remoteQueryField,
shouldFetch: true,
isMutation: true,
shouldBatch: true,
})
}

Expand Down Expand Up @@ -157,6 +159,7 @@ export class RemoteRequestDataSource {
operationHash,
operationVariables,
requestPolicy: 'network-only',
shouldBatch: params.shouldBatch,
}))
.then((result) => {
const toPushDefinition = this.#operationRegistryPushToFrontend.get(operationHash)
Expand Down Expand Up @@ -292,6 +295,7 @@ export class RemoteRequestDataSource {
shouldFetch: shouldEagerFetch,
remoteQueryField: fieldConfig.remoteQueryField,
isMutation: false,
shouldBatch: true,
})
})
}
Expand Down
13 changes: 12 additions & 1 deletion packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts
Expand Up @@ -105,6 +105,7 @@ async function makeE2ETasks () {
let ctx: DataContext
let testState: Record<string, any> = {}
let remoteGraphQLIntercept: RemoteGraphQLInterceptor | undefined
let remoteGraphQLInterceptBatchHandler: RemoteGraphQLInterceptor | undefined
let scaffoldedProjects = new Set<string>()

const cachedCwd = process.cwd()
Expand Down Expand Up @@ -182,6 +183,7 @@ async function makeE2ETasks () {
sinon.reset()
sinon.restore()
remoteGraphQLIntercept = undefined
remoteGraphQLInterceptBatchHandler = undefined

const fetchApi = ctx.util.fetch

Expand Down Expand Up @@ -213,7 +215,11 @@ async function makeE2ETasks () {

operationCount[operationName ?? 'unknown']++

if (remoteGraphQLIntercept) {
if (operationName === 'batchTestRunnerExecutionQuery' && remoteGraphQLInterceptBatchHandler) {
// const toSettle = _.mapValues(result.data, (key, val) => {
// /^_(?:\d+)_(.*?)%/.test(key)
// })
} else if (remoteGraphQLIntercept) {
try {
result = await remoteGraphQLIntercept({
operationName,
Expand Down Expand Up @@ -278,6 +284,11 @@ async function makeE2ETasks () {

return null
},
__internal_remoteGraphQLInterceptBatched (fn: string) {
remoteGraphQLInterceptBatchHandler = new Function('console', 'obj', 'testState', `return (${fn})(obj, testState)`).bind(null, console) as RemoteGraphQLInterceptor

return null
},
async __internal_addProject (opts: InternalAddProjectOpts) {
if (!scaffoldedProjects.has(opts.projectName)) {
await __internal_scaffoldProject(opts.projectName)
Expand Down
10 changes: 10 additions & 0 deletions packages/frontend-shared/cypress/e2e/support/e2eSupport.ts
Expand Up @@ -133,6 +133,10 @@ declare global {
* Gives the ability to intercept the remote GraphQL request & respond accordingly
*/
remoteGraphQLIntercept: typeof remoteGraphQLIntercept
/**
* Gives the ability to intercept the remote GraphQL request & respond accordingly
*/
remoteGraphQLInterceptBatched: typeof remoteGraphQLInterceptBatched
/**
* Removes the sinon spy'ing on the remote GraphQL fake requests
*/
Expand Down Expand Up @@ -444,6 +448,12 @@ function remoteGraphQLIntercept (fn: RemoteGraphQLInterceptor) {
})
}

function remoteGraphQLInterceptBatched (fn: RemoteGraphQLInterceptor) {
return logInternal('remoteGraphQLInterceptBatched', () => {
return taskInternal('__internal_remoteGraphQLInterceptBatched', fn.toString())
})
}

type Resolved<V> = V extends Promise<infer U> ? U : V

/**
Expand Down

0 comments on commit 2517f8b

Please sign in to comment.