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.

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

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Aug 30, 2023

  • For a saved execution, we write to disk binary data and metadata. These two are only ever deleted via POST /executions/delete. No marker file, so untouched by pruning.
  • For an unsaved execution, we write to disk binary data, binary data metadata, and a marker file at /meta. We later delete all three during pruning.
  • The third flow is legacy. Currently, if the execution is unsaved, we actually store it in the DB while running the workflow and immediately after the workflow is finished during the onWorkflowPostExecute() hook we delete that execution, so the second flow applies. But formerly, we did not store unsaved executions in the DB ("ephemeral executions") and so we needed to write a marker file at /persistMeta so that, if the ephemeral execution crashed after the step where binary data was stored, we had a way to later delete its associated dangling binary data via a second pruning cycle, and if the ephemeral execution succeeded, then we immediately cleaned up the marker file at /persistMeta during the onWorkflowPostExecute() hook.

This creation and cleanup at /persistMeta is still happening, but this third flow no longer has a purpose, as we now store unsaved executions in the DB and delete them immediately after. Hence the third flow can be removed.

@github-actions
Copy link
Contributor

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

  • If fixing bug, added test to cover scenario.
  • If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

  • Added unit tests to cover new or updated functionality.

Make sure to check off this list before asking for review.

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/improvement New feature or request labels Aug 30, 2023
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage: 16.66% and project coverage change: -0.01% ⚠️

Comparison is base (e1922f7) 32.10% compared to head (c3bd6e7) 32.10%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7046      +/-   ##
==========================================
- Coverage   32.10%   32.10%   -0.01%     
==========================================
  Files        3186     3186              
  Lines      195780   195756      -24     
  Branches    21355    21351       -4     
==========================================
- Hits        62858    62841      -17     
+ Misses     131888   131881       -7     
  Partials     1034     1034              
Files Changed Coverage Δ
packages/cli/src/InternalHooks.ts 5.30% <ø> (-0.40%) ⬇️
packages/cli/src/config/schema.ts 37.50% <ø> (ø)
packages/cli/src/controllers/e2e.controller.ts 0.00% <ø> (ø)
packages/core/src/BinaryDataManager/FileSystem.ts 62.50% <ø> (+0.37%) ⬆️
packages/core/src/BinaryDataManager/index.ts 36.97% <ø> (+0.90%) ⬆️
...kages/nodes-base/nodes/MongoDb/GenericFunctions.ts 0.00% <0.00%> (ø)
packages/nodes-base/nodes/MongoDb/MongoDb.node.ts 0.00% <0.00%> (ø)
...ages/nodes-base/nodes/MongoDb/MongoDbProperties.ts 0.00% <0.00%> (ø)
...editor-ui/src/views/SettingsCommunityNodesView.vue 47.56% <50.00%> (+0.19%) ⬆️
packages/cli/src/Mfa/totp.service.ts 90.00% <100.00%> (ø)
... and 1 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@netroy netroy self-requested a review August 31, 2023 13:37
@@ -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.

@ivov ivov merged commit 8cd4db0 into master Aug 31, 2023
18 of 19 checks passed
@ivov ivov deleted the simplify-marking-logic-in-binary-data-manager branch August 31, 2023 14:02
@github-actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@janober
Copy link
Member

janober commented Sep 6, 2023

Got released with n8n@1.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/improvement New feature or request Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants