-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add buffer configurations to readers and writers #40
Conversation
change comment
Add TombRecords and size in sstable metadata to handle the GC process with a Threshold in size or record nb.
simpledb/flush.go
Outdated
@@ -50,6 +51,7 @@ func executeFlush(db *DB, flushAction memStoreFlushAction) error { | |||
err = memStoreToFlush.FlushWithTombstones( | |||
sstables.WriteBasePath(writePath), | |||
sstables.WithKeyComparator(db.cmp), | |||
sstables.WriteBufferSizeBytes(16*1024), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think that it's enough no ? Should make some benchmark. But 4Mb for each flush is for my case a huge waste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to make it configurable as well, but 16k is absolutely fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should have a global config in the simpledb struct. I will do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. Two options, one for reading, one for writing.
Could you review this PR please ? |
Sure, will get back to you by EoD :) |
func TestReaderInitNoPath(t *testing.T) { | ||
_, err := NewFileReader() | ||
assert.Equal(t, errors.New("NewFileReader: either os.File or string path must be supplied, never both"), err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry to be pedantic 🤣 but can you add another where to supply both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
one little test missing, can you squash your commits with a meaningful commit message as well? |
I've squashed my commit since the last merge. I thinks that's good now. |
it is, thank you! |
could you please tag a new version ? |
yep, will do in the evening. |
@sebheitzmann done! |
No description provided.