Skip to content

Conversation

@Arsnael
Copy link
Contributor

@Arsnael Arsnael commented Jan 7, 2026

Replacing #2899

Copy link
Member

@quantranhong1999 quantranhong1999 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

Great as a first step!

I propose to wait to have the purge working on top of the v2 (in addition to the v1) in order not to ship broken code

Thanks @Arsnael for proposing this change.

@Arsnael
Copy link
Contributor Author

Arsnael commented Jan 7, 2026

I propose to wait to have the purge working on top of the v2 (in addition to the v1) in order not to ship broken code

Alright will add the purge refactoring on this PR then. Thanks for the feedback

EDIT: I think technically the purge with old messages works as demonstrated in the adapted test. It's just not doing it for the new single bucket yet.

@chibenwa
Copy link
Contributor

chibenwa commented Jan 7, 2026

EDIT: I think technically the purge with old messages works as demonstrated in the adapted test

Yes I know.

But not with the new.

Hence my remark.

@chibenwa
Copy link
Contributor

chibenwa commented Jan 8, 2026

CF remarks on #2894

While there's no objection continuing this work so that it's ready I would actually like to also get the "S3 contained alternative" and potentially bring up a debate on the ML on the two implementations.

@Arsnael
Copy link
Contributor Author

Arsnael commented Jan 8, 2026

CF remarks on #2894

I've seen yes. I've been thinking worth continuing in case, as the remarks were not widely discussed and validated with the community yet and might require more work.

@Arsnael
Copy link
Contributor Author

Arsnael commented Jan 9, 2026

Might have a few bits left:

  • probably adding a few integration tests to the existing ones
  • likely adding some additional infos? like number of blobs deleted for example for the single bucket?
  • then I still have an issue with a test, not sure what approach I should use

In DeletedMessageVaultIntegrationTest , the test vaultDeleteShouldDeleteAllMessagesHavingSameBlobContent .

Here it fails, it doesn't work anymore. I guess maybe because same blob content message used to have the same blobId, but maybe here with the different time prefix, it's not the case? Is that case still relevant or not with this new design is what I'm wondering

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