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

levels: don't sync directory in memory mode #171

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GanZiheng
Copy link
Contributor

@GanZiheng GanZiheng commented Jun 27, 2022

This will cause problem when performing compaction in memory mode, since we will never return tables built by compaction successfully due to the sync_dir returning error with empty path input.

Signed-off-by: GanZiheng ganziheng98@gmail.com

Signed-off-by: GanZiheng <ganziheng98@gmail.com>
@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #171 (1b8c19d) into master (a4e06f1) will increase coverage by 0.09%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage   89.54%   89.63%   +0.09%     
==========================================
  Files          39       39              
  Lines        8739     8741       +2     
==========================================
+ Hits         7825     7835      +10     
+ Misses        914      906       -8     

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

What's the error

@GanZiheng
Copy link
Contributor Author

What's the error

If we put entries enough to cause compaction in memory mode, we would get this error.

I tried to add test for this, but it will result in OOM in memory sanitizer test.

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

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