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

Adding ChunkManager implementation #206

Closed
wants to merge 2 commits into from

Conversation

AnatolyPopov
Copy link
Contributor

@AnatolyPopov AnatolyPopov commented May 7, 2023

Adding ChunkManager implementation that allows to fetch a specific chunk and return as decrypted and decompressed stream of bytes.

@AnatolyPopov AnatolyPopov requested a review from a team as a code owner May 7, 2023 14:09
Copy link
Contributor

@jeqo jeqo left a comment

Choose a reason for hiding this comment

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

Thanks @AnatolyPopov.
I think we need some tests for this component as well.

Also, some general comments:
The getChunk implementation (decrypt, decompress) corresponds to the uploading logic (compress, encrypt). These should be on the same place.
I see two alternatives to do this:

  • URSM implementing this interface
  • Extending this class to handle all interaction between Kafka TS ops and ObjectStorageFactory:
    • In this case, I'd rename the class to ChunkedSegmentManager or similar, and extend it to fetchChunk, uploadSegment (including upload log, indexes, and manifest) and deleteSegment.

I'm leaning towards the second option. Though, regardless, agreeing on either should help to scope this and the following PRs better. wdyt?

@AnatolyPopov
Copy link
Contributor Author

@jeqo I totally agree regarding regarding your comment about having encryption/decryption and compression/decompression in the same place and was thinking about refactoring but I believe this can be addressed in other PRs and requires some discussions. For now I would proceed almost as is to finally get the first version working and then refactor this.

Copy link
Contributor

@jeqo jeqo left a comment

Choose a reason for hiding this comment

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

Sure, let's review it after URSM implementation is merged. Creating #207 to follow up.

@jeqo
Copy link
Contributor

jeqo commented May 8, 2023

@AnatolyPopov could you squash commits? I can merge it right after

@AnatolyPopov
Copy link
Contributor Author

@jeqo Done

feat: adding ChunkManager implementation
@jeqo jeqo deleted the anatolii/chunk-manager branch May 8, 2023 13:58
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.

2 participants