Delete op #6
Delete op #6
Conversation
.github/workflows/build.yml
Outdated
| - 'final' | ||
| pull_request: | ||
| branches: | ||
| - 'final' |
There was a problem hiding this comment.
I feel it is better if we keep the same build file across branches. We don't need to deal with merge conflicts etc
something like:
| - 'final' | |
| pull_request: | |
| branches: | |
| - 'final' | |
| - 'master' | |
| - 'final' | |
| pull_request: | |
| branches: | |
| - 'master' | |
| - 'final' |
| if err != nil { | ||
| panic("read error") | ||
| return "", ErrReadFailed |
There was a problem hiding this comment.
thanks for adding error handling. I had omitted them for simplicity.
| err := r.EncodeKV(buf) | ||
| if err != nil { | ||
| log.Fatalf("error in encoding the value %v", err) | ||
| return ErrEncodingFailed |
There was a problem hiding this comment.
no need of log.Fatal when you are returning the error
disk_store.go
Outdated
| if err != nil { | ||
| log.Fatalf("error while reading the value: %v", err) |
There was a problem hiding this comment.
here and at 237, no need to log.Fatal when it has break statement next
a better API would be to make initKeyDir return an error and log.fatal in the caller
format.go
Outdated
| // └───────────────┴──────────────┴────────────────┘ | ||
| // ┌───────────────┬──────────────┬────────────────┬────────────────┐ | ||
| // │ timestamp(4B) │ key_size(4B) | value_size(4B) │ tombstone(2B) │ | ||
| // └───────────────┴──────────────┴────────────────┴────────────────┘ |
There was a problem hiding this comment.
why 2 bytes for tombstone though, sounds like a lot to me
There was a problem hiding this comment.
because, bigger the size, bigger the storage space we would require.
There was a problem hiding this comment.
I would suggest allocating one byte and call it Meta. The first bit would be set to 1 for tombstones
There was a problem hiding this comment.
later we can introduce compression for values and use the second bit and so on.
There was a problem hiding this comment.
also, meta can be the first field:
Meta (2B) | timestamp(4B) │ key_size(4B) | value_size(4B)
disk_store.go
Outdated
| buf := bytes.NewBuffer(make([]byte, headerSize)) | ||
| err := r.EncodeKV(buf) |
There was a problem hiding this comment.
why not make EncodeKV to take care of the buffer too? Here is the API I suggest:
buf, err := r.EncodeKV()
Internally, it will allocate the buffer and do whatever is necessary
There was a problem hiding this comment.
yeah, I followed the standard practice of preallocating buffer in the caller itself and letting the function fill it, much like Read works of the Reader interface. Also this avoids the sharing up construct.
There was a problem hiding this comment.
Also this avoids the sharing up construct.
can you elaborate?
There was a problem hiding this comment.
As far as I understand stack and heap allocations in golang, sharing pointers or references up(from the callee) escapes to heap right. So even though we are out of the callee, go has to keep tracking that value on the heap in case you work on it further.
https://youtu.be/ZMZpH4yT7M0?t=930
In this case, since we are pre allocating byte slice, if it's small enough to be allocated on stack, go will otherwise it will escape to heap anyways.
disk_store.go
Outdated
| return ErrEncodingFailed | ||
| } | ||
| d.write(buf.Bytes()) | ||
| size := headerSize + h.KeySize + h.ValueSize |
There was a problem hiding this comment.
how about making size as property of Record, like Record.Size()? It has all the necessary information
disk_store.go
Outdated
| h := &Header{} | ||
| h.DecodeHeader(header) |
There was a problem hiding this comment.
instead of this, I would prefer an API like:
h, err := NewHeader(headerBytes)
disk_store_test.go
Outdated
| //check for deletion | ||
| for _, dkeys := range deletedKeys { | ||
| actualVal, err := store.Get(dkeys) | ||
| if actualVal != "" && errors.Is(err, ErrKeyNotFound) { |
There was a problem hiding this comment.
if actualVal != "" || errors.Is(err, ErrKeyNotFound)
Better would be splitting this into two conditions, so that we can log the actual error condition:
if actualVal != "" {}
if !errors.Is(err, ErrKeyNotFound) {}
There was a problem hiding this comment.
I added & cause Im checking only for the keys in deletedKeys list explicitly. But I get your point, making the change.
| ErrSeekFailed = errors.New("see fail: failed to seek to the correct offset") | ||
| ErrReadFailed = errors.New("read fail: failed to read data from disk") | ||
| ErrEncodingFailed = errors.New("encoding fail: failed to encode kv record") | ||
| ) |
There was a problem hiding this comment.
this is nice, all the errors are at one place
|
Thank you 🎉 |
No description provided.