feat: delete layer (MAPCO-1159) - #33
Conversation
|
🎫 Related Jira Issue: MAPCO-1159 |
| const reason = describeError(error); | ||
| this.logger.debug({ msg: 'Failed to delete file', path: join(storageTarget, relativePath), reason, error }); | ||
| failures.push({ path: relativePath, reason }); | ||
| for (const relativePaths of getChunk(paths, this.fsConfig.delete.batchSize)) { |
There was a problem hiding this comment.
The batch handling is occurring in the strategy level(tileDeletionStrategy):
const flushedCount = pendingBatches.reduce((sum, b) => sum + b.length, 0); const results = await Promise.allSettled(pendingBatches.map(async (batch) => provider.delete(batch, storageTarget)));
So either remove getChunk() function or remove batch handling from tileDeletionStrategy
| "delete": { | ||
| "batchSize": 1000 | ||
| }, | ||
| "basePath": "/tiles" |
There was a problem hiding this comment.
basePath should be configured per strategy:
For example when deleting tiles we would want the basePathe to be raster/artifacts/tiles.
And when we delete legends or thumbnails raster/artifacts/thumbnails or raster/artifacts/legends.
Currently in tilesDeletionStrategy the tiles lcoation and the bucket is driven from the cleaner config and not by passing it threw the params (we discussed that we should do it later in another ticket and different scope).
| {{- end }} | ||
| {{- if has "FS" $storage.cleanupStorageProviders }} | ||
| FS_DELETE_BATCH_SIZE: {{ $fs.delete.batchSize | default 1000 | quote }} | ||
| FS_BASE_PATH: {{ $tilesFSBasePath | quote }} |
There was a problem hiding this comment.
Same here you mapped FS_BASE_PATH to tilesFSBasePath.
There was a problem hiding this comment.
I suggest to keep FS_BASE_PATH and his value should be the $internalPvc.mountPath
and for each artifact we should define is own subPath like raster/artifacts/legend, raster/artifacts/tiles, raster/artifacts/thumbnails and etc...,
| ) { | ||
| this.fsConfig = this.config.get('storage.fs') as unknown as FsConfig; | ||
| if (this.fsConfig.delete.batchSize <= 0) throw new ConfigurationError('Deletion batch size must be greater than 0'); | ||
| this.basePath = resolveAbsolutePath(this.fsConfig.basePath); |
There was a problem hiding this comment.
Here it's looks like you are referring to the basePath but actually :
cleaner/helm/templates/configmap.yaml
Line 68 in ba892c6
so the "basePath" is acttualy referrs to tilesPath and when we would like to delete other resources it will not work
| } | ||
| } | ||
|
|
||
| private checkPathTraversal(paths: string[]): boolean { |
There was a problem hiding this comment.
Nice one, but keep in mind that the entire paths deletion could be failed, all because one bad path,
Consider deleting only the paths that pass the check and exclude the once who failed and maybe logged them so we can later investigate the issue.
| private readonly config: ConfigType, | ||
| private readonly logger: Logger | ||
| ) { | ||
| this.fsConfig = this.config.get('storage.fs') as unknown as FsConfig; |
There was a problem hiding this comment.
those checks should be done in higher level and not in the fsStorageProvider ctor
| failures: DeleteFailure[]; | ||
| } | ||
|
|
||
| export type StorageProvider = DeleteStoredResourcesParams['storageProvider']; |
There was a problem hiding this comment.
Use Storgae type instead of DeleteStoredResourcesParams
| failures: DeleteFailure[]; | ||
| } | ||
|
|
||
| export type StorageProvider = DeleteStoredResourcesParams['storageProvider']; |
There was a problem hiding this comment.
| export type StorageProvider = DeleteStoredResourcesParams['storageProvider']; | |
| export type StorageProvider = Storage['storageProvider']; |
| } | ||
|
|
||
| export interface IStorageProvider { | ||
| export interface DeleteResourcesResult { |
There was a problem hiding this comment.
Why are you creating new type and not keeping similar signature to the delete function?
| * - S3: storageTarget = bucket name; paths are root paths to resource(s) | ||
| * - FS: storageTarget = base directory; paths are relative paths from mounted dir | ||
| */ | ||
| deleteResources: (deleteStoredResourcesParams: Extract<DeleteStoredResourcesParams, { storageProvider: T }>) => Promise<DeleteResourcesResult>; |
There was a problem hiding this comment.
| deleteResources: (deleteStoredResourcesParams: Extract<DeleteStoredResourcesParams, { storageProvider: T }>) => Promise<DeleteResourcesResult>; | |
| deleteResources: (deleteStoredResourcesParams: Extract<DeleteStoredResourcesParams, { storageProvider: T }>) => Promise<DeleteFailure[]>; |
| const failures: DeleteFailure[] = []; | ||
|
|
||
| // The configured batch size may exceed the DeleteObjects API limit, so re-chunk defensively here. | ||
| for (const keys of getChunk(paths, S3_DELETE_OBJECTS_MAX_KEYS)) { |
There was a problem hiding this comment.
You are using getChunk inf unction that called deleteChunk- which means you are already dealing with chunk of paths - no need to extract more chunks.
| for await (const pageOfObjects of s3Objects) { | ||
| this.logger.debug({ | ||
| msg: `Received a batch of ${pageOfObjects.length} objects to delete`, | ||
| pageOfObjects, |
There was a problem hiding this comment.
Consider log sample of the pageOfObjects instead of logging it all.
| totalFailedObjectsCount, | ||
| err, | ||
| }); | ||
| throw err; |
There was a problem hiding this comment.
Suggestion: instead of throwing return the paths who failed.
| path, | ||
| pageSize: this.s3Config.delete.batchSize, | ||
| }); | ||
| const keys = pageOfObjects.map((obj) => obj.Key).filter((key): key is string => key !== undefined && this.matchesTarget(key, path)); |
There was a problem hiding this comment.
Should we logged or keep any record of the filtered out keys?
2 participants
this PR also handles MAPCO-10813