Skip to content

RUMM-2235: Rework file persistence layer#947

Merged
0xnm merged 1 commit into
feature/sdkv2from
nogorodnikov/rumm-2235/rework-file-persistence-layer
Jun 17, 2022
Merged

RUMM-2235: Rework file persistence layer#947
0xnm merged 1 commit into
feature/sdkv2from
nogorodnikov/rumm-2235/rework-file-persistence-layer

Conversation

@0xnm

@0xnm 0xnm commented Jun 13, 2022

Copy link
Copy Markdown
Member

What does this PR do?

This change makes some re-work of the file persistence layer to answer the new requirements:

  • We have now 2 file formats to maintain: TLV format with meta+data types used for events storage and plain file format used for metadata storage. Reading is also different for these 2: either we return single data chunk, or multiple data chunks.
  • File move/delete operations shouldn't depend on the file content/format.
  • Encryption should be supported by both file formats as well.

To support these requirements the following was done in this PR:

  • Moved out file move/delete logic into a dedicated class FileMover (not the best name, I know, but FileOrchestrator was already taken).
  • Introduced different interfaces for file writing (the same interface for both file formats) and file reading (2 different interfaces, because Kotlin/Java doesn't support method overload for different return types).
  • Moved metadata file handling responsibility into FileOrchestrator - this gives an advantage for the proper file cleanup once batch is dropped, expired or simply disk space should be freed.
  • Introduced a dedicated class for writing data in the plain format (for metadata cases).

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@0xnm
0xnm marked this pull request as ready for review June 14, 2022 07:22
@0xnm
0xnm requested a review from a team as a code owner June 14, 2022 07:22

@xgouchet xgouchet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

// is requested with granted orchestrator (due to consent change). Not an issue, because
// batch file should be migrated to the same folder, but leaving this debug point
// just in case.
internalLogger.debugWithTelemetry(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice 👍

uploader.upload(batch)
handler.delete(it)
fileMover.delete(it)
val metaFile = fileOrchestrator.getMetadataFile(it)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

where is this data uploaded if you delete it here ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The data is uploaded few lines above (uploader.upload(batch)), basically the logic is the same, I just added metadata deletion.

But I see your concern: metadata is requested for the file which is deleted and while it is ok to have (File object stays there still and we just use its path to get associated metadata file), I swapped the lines - now metadata file is requested before batch file deletion, which makes the operation more clear.

@0xnm
0xnm force-pushed the nogorodnikov/rumm-2235/rework-file-persistence-layer branch from fea0141 to 9b09887 Compare June 16, 2022 08:38
@0xnm
0xnm merged commit 8e9eab1 into feature/sdkv2 Jun 17, 2022
@0xnm
0xnm deleted the nogorodnikov/rumm-2235/rework-file-persistence-layer branch June 17, 2022 07:30
@xgouchet xgouchet added this to the 1.16.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants