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

feat: added interval based wal synchronization #44

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

hitesh22rana
Copy link
Contributor

@hitesh22rana hitesh22rana commented Jan 19, 2025

Feature: #13 (comment)
Asynchronously sync the data to disk at configurable time interval.

@hitesh22rana hitesh22rana mentioned this pull request Jan 19, 2025
@hitesh22rana
Copy link
Contributor Author

There seems to be some failed tests.
Let me take a look and revert to you 🙇🏼

@hitesh22rana hitesh22rana force-pushed the feat/interval-based-sync branch from 7471233 to aa0c176 Compare January 20, 2025 01:32
@hitesh22rana hitesh22rana force-pushed the feat/interval-based-sync branch from aa0c176 to 1a6a3cd Compare January 20, 2025 01:40
@hitesh22rana
Copy link
Contributor Author

hitesh22rana commented Jan 20, 2025

@roseduan PTAL 🙏🏼,
Tests seems to be fixed now.

Reasons of test failure in first place
since, we are already closing wal via destroyWAL function in wal_test.go, in some of the test cases it was also trying to invoke the Close() explicitly which was causing panic.

Explicit check is now added in Close() function in wal.go to only close the channel if it is not already close.

@hitesh22rana
Copy link
Contributor Author

Hi @roseduan, just a friendly reminder to PTAL when you get a chance 🙏🏼.

The tests seem stable now, and I've addressed the root cause of the previous failures (detailed above). Let me know if there's anything else you'd like me to adjust or clarify.
Thank you!

@roseduan
Copy link
Contributor

LGTM

@roseduan roseduan merged commit 9f1a618 into rosedblabs:main Jan 26, 2025
2 checks passed
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