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: make write method object safe in WalManager #159

Merged
merged 5 commits into from
Jul 30, 2022

Conversation

ygf11
Copy link
Contributor

@ygf11 ygf11 commented Jul 28, 2022

Which issue does this PR close?

Part of #72

Rationale for this change

Make WalManager object safe, so we can use Arc in other place.

What changes are included in this PR?

  1. Remove LogWriter, and move write to WalManager.
  2. Remove generic and associate params in Payload.

Are there any user-facing changes?

No

How does this change test

The refactor is tested by the compiler.

@ygf11 ygf11 changed the title refactor: make write method object safe in WalManager [WIP] refactor: make write method object safe in WalManager Jul 28, 2022
@ygf11 ygf11 marked this pull request as ready for review July 28, 2022 09:09
@ygf11 ygf11 changed the title [WIP] refactor: make write method object safe in WalManager refactor: make write method object safe in WalManager Jul 28, 2022
@ygf11
Copy link
Contributor Author

ygf11 commented Jul 28, 2022

@waynexia PTAL.

@waynexia
Copy link
Member

Fancinating! I plan to review this later this day 🤩

@waynexia waynexia added feature New feature or request A-analytic-engine Area: Analytic Engine labels Jul 29, 2022
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR. It looks good to me 👍

For overhead introduced here (Payload becomes a trait object) we can find some way to alleviate it like changing the write interface to directly accept encoded bytes. But this is beyond the scope of the refactor this time. And we are lacking of benchmark so don't know how much regression is brought actually.

I'm going to merge tomorrow if there is no other review.

@ygf11
Copy link
Contributor Author

ygf11 commented Jul 29, 2022

Thanks for your suggestions.

For overhead introduced here (Payload becomes a trait object) we can find some way to alleviate it like changing the write interface to directly accept encoded bytes.

I feel the same. we can decouple encoding from write of WalManager. I plan to create a issue about this later, we can talk deeply there :D

But this is beyond the scope of the refactor this time. And we are lacking of benchmark so don't know how much regression is brought actually.

Yes, we need a bench.

@waynexia waynexia merged commit 5bfb577 into apache:main Jul 30, 2022
@ygf11 ygf11 deleted the wal branch August 1, 2022 03:45
chunshao90 pushed a commit to chunshao90/ceresdb that referenced this pull request May 15, 2023
* refactor: make write method object safe in WalManager

* fix: clean unused code

* fix: remove unused trait impl

* fix: remove generic param in Writer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-analytic-engine Area: Analytic Engine feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants