Raft handle region broken #28
Conversation
| UInt32 size; | ||
| UInt64 version; | ||
| UInt64 offset; | ||
| UInt64 checksum; |
There was a problem hiding this comment.
Is is UInt32 enough for checksum?
There was a problem hiding this comment.
UInt64 is already the smallest, unless we use another hash function instead of Cityhash provided by CH.
| UInt32 level; | ||
| UInt32 size; | ||
| UInt64 version; | ||
| UInt64 offset; |
There was a problem hiding this comment.
If Page size is fixed, we can use page-index instead of real offset
There was a problem hiding this comment.
The fixed page size design is deprecated.
| struct PageCache | ||
| { | ||
| PageFileId file_id; | ||
| UInt32 level; |
There was a problem hiding this comment.
Use to support GC. If we have three pages are selected to GC, (1, 0), (2, 0), (3, 1), then the merged file will be (3, 2).
| #define O_DIRECT 00040000 | ||
| #endif | ||
|
|
||
| template <bool read, bool must_exist = true> |
There was a problem hiding this comment.
Open a file is a heavy system call, moving 'read' and 'must_exist' from function args to template args is too aggressive and I don't think it helps.
|
|
||
| void seekFile(int fd, off_t pos, const std::string & path) | ||
| { | ||
| ProfileEvents::increment(ProfileEvents::Seek); |
There was a problem hiding this comment.
That would be a lot of event? I don't know
There was a problem hiding this comment.
That is fine. It just do a atomic increment.
| throwFromErrno("Cannot fsync " + path, ErrorCodes::CANNOT_FSYNC); | ||
| } | ||
|
|
||
| void seekFile(int fd, off_t pos, const std::string & path) |
There was a problem hiding this comment.
I can't find where are using seekFile, but for me, I will always prefer 'pread/pwrite' than 'read/write'. ($>man pread)
There was a problem hiding this comment.
Yes, this method is useless and should be removed.
There was a problem hiding this comment.
My mistake, this method is used after a file is opened for write. Currently writing does not use ::pwrite(), but ::write() because we only do append here.
| throwFromErrno("Cannot seek through file " + path, ErrorCodes::CANNOT_SEEK_THROUGH_FILE); | ||
| } | ||
|
|
||
| void writeFile(int fd, const char * data, size_t to_write, const std::string & path) |
There was a problem hiding this comment.
1, If the caller of writeFile had not consider thread-safe, this is very dangerous.
2, If the caller of writeFile had consider thread-safe and use some sort of locker, this function will be slow, case block is not big and all the writing will be queued to single-thread mode.
There was a problem hiding this comment.
By design we only support one thread write for now. Multi-thread write will be support later, by introducing multiple write files.
|
LGTM |
flowbehappy
force-pushed
the
raft-handle-region-broken
branch
from
d81f177
to
df0b091
Compare
This pr is related to issues: