-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: implement naive cache solution #17
Conversation
AnguIar
commented
Dec 27, 2021
Question | Answer |
---|---|
New feature | ✔ |
This pull request introduces 1 alert when merging 355d6cf into f9d5dc0 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 14d69da into f9d5dc0 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging bacc684 into f9d5dc0 - view on LGTM.com new alerts:
|
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.
👍 good job
src/clients/jobManagerClient.ts
Outdated
import { ICreateJobBody, ICreateJobResponse, IJobCreationResponse, IWorkerInput } from '../common/interfaces'; | ||
import booleanEqual from '@turf/boolean-equal'; | ||
import bboxPolygon from '@turf/bbox-polygon'; | ||
import { ICreateJobBody, IJobResponse, IUpdateJobBody, OperationStatus } from '@map-colonies/mc-priority-queue'; |
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.
remove old interfaces from /common/interfaces
src/clients/jobManagerClient.ts
Outdated
@@ -19,26 +28,31 @@ export class JobManagerClient extends HttpClient { | |||
this.tilesTaskType = config.get<string>('workerTypes.tiles.taskType'); | |||
} | |||
|
|||
public async createJob(data: IWorkerInput, layer: LayerMetadata): Promise<IJobCreationResponse> { | |||
public async updateJob(jobId: string, payload: IUpdateJobBody<IJobParameters>): Promise<void> { |
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.
as you have inserted the "mc-priority-queue" package use the built in "jobManagerClient"
use https://github.com/MapColonies/mc-priority-queue/blob/master/src/jobManagerClient.ts#L137
src/clients/jobManagerClient.ts
Outdated
await this.put(updateJobUrl, payload); | ||
} | ||
|
||
public async createJob(data: IWorkerInput): Promise<ICreateJobResponse> { |
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.
as you have inserted the "mc-priority-queue" package use the built in "jobManagerClient"
use https://github.com/MapColonies/mc-priority-queue/blob/master/src/jobManagerClient.ts#L124
src/clients/jobManagerClient.ts
Outdated
return undefined; | ||
} | ||
|
||
private async getJobs(queryParams: IFindJob): Promise<JobResponse[] | undefined> { |
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.
option a: add "/jobs Get" method to pc-priority-que "JobManagerClient" and use it
option b: inherit the "JobManagerClient" and add this method localy
} | ||
} | ||
|
||
private async checkForProcessing(jobParams: JobDuplicationParams, addedCallbackUrls: string[]): Promise<ICreateJobResponse | undefined> { |
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.
rename to check for inProgress?
@@ -14,7 +14,7 @@ interface IServerConfig { | |||
port: string; | |||
} | |||
|
|||
const serverConfig = get<IServerConfig>('server'); | |||
const serverConfig = config.get<IServerConfig>('server'); |
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.
add p to boiler plate?
@@ -1,88 +1,285 @@ | |||
/* eslint-disable */ | |||
import { LayerMetadata } from '@map-colonies/mc-model-types'; | |||
import { IWorkerInput } from '../../src/common/interfaces'; | |||
import { IJobResponse, OperationStatus } from '@map-colonies/mc-priority-queue'; |
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.
fix prettier
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.
👍
@@ -79,28 +81,39 @@ export class CreatePackageManager { | |||
return processingExists; | |||
} | |||
|
|||
// For race condition |
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.
race-condition logic need to change according to the design
@@ -1,9 +1,10 @@ | |||
import jsLogger from '@map-colonies/js-logger'; | |||
import { OperationStatus } from '@map-colonies/mc-priority-queue'; | |||
import { JobManagerClient } from '../../../src/clients/jobManagerClient'; | |||
import { JobManagerWrapper } from '../../../src/clients/jobManagerWrapper'; | |||
import { IJobParameters, ITaskParameters } from '../../../src/common/interfaces'; |
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.
fix lint errors