Fix flushing logic and add functional test case. #113
Conversation
persist/fs/writer.go
Outdated
| @@ -27,18 +27,22 @@ var ( | |||
|
|
|||
| // segmentWriter is responsible for writing segments to filesystem. | |||
| type segmentWriter interface { | |||
| // Open opens the writer. | |||
| Open(opts writerOpenOptions) error | |||
| // Start persisting data to disk. | |||
There was a problem hiding this comment.
nit: Should be a sentence, e.g., Start starts persisting a segment.
persist/fs/writer.go
Outdated
| // Start persisting data to disk. | ||
| Start(opts writerStartOptions) error | ||
|
|
||
| // Finish persisting data to disk. |
There was a problem hiding this comment.
Finish finishes persisting a segment and performs cleanups as necessary.
storage/database.go
Outdated
| return err | ||
| } | ||
|
|
||
| if err := d.Options().PersistManager().Close(); err != nil { |
There was a problem hiding this comment.
This may need to wait till the current ticking finishes (which may perform a flush). Is that the case at the moment?
There was a problem hiding this comment.
On a second thought this probably should just be removed, as closing the manager will cause the it to panic if it's in the middle of a flush, and all closing does is just setting some nil pointers (which doesn't matter much if the database is shutting down).
There was a problem hiding this comment.
Removing the close completely.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestFlush(t *testing.T) { |
There was a problem hiding this comment.
Fine to leave this here for now, but this looks like an integration test rather than a unit test (which typically tests local functionalities than performing an end-to-end test).
There was a problem hiding this comment.
I put it here to access internal structs to force a flush.
Fixes #107 and #108.
Also adds a functional test case that repros both issues.