Skip to content
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.

Sign up for GitHub

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

refactor(core): Simplify marking logic in binary data manager (no-changelog) #7046

Merged
merged 1 commit into from
Aug 31, 2023
Merged

refactor(core): Simplify marking logic in binary data manager (no-changelog) #7046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions packages/cli/src/InternalHooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Service } from 'typedi';
import { snakeCase } from 'change-case';
import { BinaryDataManager } from 'n8n-core';
import type {
AuthenticationMethod,
ExecutionStatus,
Expand Down Expand Up @@ -461,8 +460,6 @@ export class InternalHooks implements IInternalHooksClass {
}),
);

await BinaryDataManager.getInstance().persistBinaryDataForExecutionId(executionId);

void Promise.all([...promises, this.telemetry.trackWorkflowExecution(properties)]);
}

Expand Down
6 changes: 0 additions & 6 deletions packages/cli/src/config/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,12 +913,6 @@ export const schema = {
env: 'N8N_BINARY_DATA_TTL',
doc: 'TTL for binary data of unsaved executions in minutes',
},
persistedBinaryDataTTL: {
format: Number,
default: 1440,
env: 'N8N_PERSISTED_BINARY_DATA_TTL',
doc: 'TTL for persisted binary data in minutes (binary data gets deleted if not persisted before TTL expires)',
},
},

deployment: {
Expand Down
53 changes: 0 additions & 53 deletions packages/core/src/BinaryDataManager/FileSystem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import type { IBinaryDataConfig, IBinaryDataManager } from '../Interfaces';
import { FileNotFoundError } from '../errors';

const PREFIX_METAFILE = 'binarymeta';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should also rename this to flagged or something like that to make this code less confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, I'll clarify names in the FS manager soon as part of the upcoming refactors.

const PREFIX_PERSISTED_METAFILE = 'persistedmeta';

const executionExtractionRegexp =
/^(\w+)(?:[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12})$/;
Expand All @@ -21,31 +20,22 @@ export class BinaryDataFileSystem implements IBinaryDataManager {

private binaryDataTTL: number;

private persistedBinaryDataTTL: number;

constructor(config: IBinaryDataConfig) {
this.storagePath = config.localStoragePath;
this.binaryDataTTL = config.binaryDataTTL;
this.persistedBinaryDataTTL = config.persistedBinaryDataTTL;
}

async init(startPurger = false): Promise<void> {
if (startPurger) {
setInterval(async () => {
await this.deleteMarkedFiles();
}, this.binaryDataTTL * 30000);

setInterval(async () => {
await this.deleteMarkedPersistedFiles();
}, this.persistedBinaryDataTTL * 30000);
}

await this.assertFolder(this.storagePath);
await this.assertFolder(this.getBinaryDataMetaPath());
await this.assertFolder(this.getBinaryDataPersistMetaPath());

await this.deleteMarkedFiles();
await this.deleteMarkedPersistedFiles();
}

async getFileSize(identifier: string): Promise<number> {
Expand All @@ -55,7 +45,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager {

async copyBinaryFile(filePath: string, executionId: string): Promise<string> {
const binaryDataId = this.generateFileName(executionId);
await this.addBinaryIdToPersistMeta(executionId, binaryDataId);
await this.copyFileToLocalStorage(filePath, binaryDataId);
return binaryDataId;
}
Expand All @@ -72,7 +61,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager {

async storeBinaryData(binaryData: Buffer | Readable, executionId: string): Promise<string> {
const binaryDataId = this.generateFileName(executionId);
await this.addBinaryIdToPersistMeta(executionId, binaryDataId);
await this.saveToLocalStorage(binaryData, binaryDataId);
return binaryDataId;
}
Expand Down Expand Up @@ -105,30 +93,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager {
return this.deleteMarkedFilesByMeta(this.getBinaryDataMetaPath(), PREFIX_METAFILE);
}

async deleteMarkedPersistedFiles(): Promise<void> {
return this.deleteMarkedFilesByMeta(
this.getBinaryDataPersistMetaPath(),
PREFIX_PERSISTED_METAFILE,
);
}

private async addBinaryIdToPersistMeta(executionId: string, identifier: string): Promise<void> {
const currentTime = new Date().getTime();
const timeAtNextHour = currentTime + 3600000 - (currentTime % 3600000);
const timeoutTime = timeAtNextHour + this.persistedBinaryDataTTL * 60000;

const filePath = this.resolveStoragePath(
'persistMeta',
`${PREFIX_PERSISTED_METAFILE}_${executionId}_${timeoutTime}`,
);

try {
await fs.access(filePath);
} catch {
await fs.writeFile(filePath, identifier);
}
}

private async deleteMarkedFilesByMeta(metaPath: string, filePrefix: string): Promise<void> {
const currentTimeValue = new Date().valueOf();
const metaFileNames = await glob(`${filePrefix}_*`, { cwd: metaPath });
Expand Down Expand Up @@ -184,19 +148,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager {
return this.deleteFromLocalStorage(identifier);
}

async persistBinaryDataForExecutionId(executionId: string): Promise<void> {
const metaFiles = await fs.readdir(this.getBinaryDataPersistMetaPath());
const promises = metaFiles.reduce<Array<Promise<void>>>((prev, curr) => {
if (curr.startsWith(`${PREFIX_PERSISTED_METAFILE}_${executionId}_`)) {
prev.push(fs.rm(path.join(this.getBinaryDataPersistMetaPath(), curr)));
return prev;
}

return prev;
}, []);
await Promise.all(promises);
}

private async assertFolder(folder: string): Promise<void> {
try {
await fs.access(folder);
Expand All @@ -213,10 +164,6 @@ export class BinaryDataFileSystem implements IBinaryDataManager {
return path.join(this.storagePath, 'meta');
}

private getBinaryDataPersistMetaPath() {
return path.join(this.storagePath, 'persistMeta');
}

private async deleteFromLocalStorage(identifier: string) {
return fs.rm(this.getBinaryPath(identifier));
}
Expand Down
6 changes: 0 additions & 6 deletions packages/core/src/BinaryDataManager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,6 @@ export class BinaryDataManager {
}
}

async persistBinaryDataForExecutionId(executionId: string): Promise<void> {
if (this.managers[this.binaryDataMode]) {
await this.managers[this.binaryDataMode].persistBinaryDataForExecutionId(executionId);
}
}

async deleteBinaryDataByExecutionIds(executionIds: string[]): Promise<void> {
if (this.managers[this.binaryDataMode]) {
await this.managers[this.binaryDataMode].deleteBinaryDataByExecutionIds(executionIds);
Expand Down
2 changes: 0 additions & 2 deletions packages/core/src/Interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ export interface IBinaryDataConfig {
availableModes: string;
localStoragePath: string;
binaryDataTTL: number;
persistedBinaryDataTTL: number;
}

export interface IBinaryDataManager {
Expand All @@ -57,7 +56,6 @@ export interface IBinaryDataManager {
deleteBinaryDataByIdentifier(identifier: string): Promise<void>;
duplicateBinaryDataByIdentifier(binaryDataId: string, prefix: string): Promise<string>;
deleteBinaryDataByExecutionIds(executionIds: string[]): Promise<string[]>;
persistBinaryDataForExecutionId(executionId: string): Promise<void>;
}

export namespace n8n {
Expand Down
2 changes: 0 additions & 2 deletions packages/core/test/NodeExecuteFunctions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ describe('NodeExecuteFunctions', () => {
availableModes: 'default',
localStoragePath: temporaryDir,
binaryDataTTL: 1,
persistedBinaryDataTTL: 1,
});

// Set our binary data buffer
Expand Down Expand Up @@ -86,7 +85,6 @@ describe('NodeExecuteFunctions', () => {
availableModes: 'filesystem',
localStoragePath: temporaryDir,
binaryDataTTL: 1,
persistedBinaryDataTTL: 1,
});

// Set our binary data buffer
Expand Down
1 change: 0 additions & 1 deletion packages/nodes-base/test/nodes/Helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,6 @@ export async function initBinaryDataManager(mode: 'default' | 'filesystem' = 'de
availableModes: mode,
localStoragePath: temporaryDir,
binaryDataTTL: 1,
persistedBinaryDataTTL: 1,
});
return temporaryDir;
}
Expand Down