Conversation
| Status Dump(const std::string &file_name); | ||
|
|
||
| private: | ||
| class Util { |
There was a problem hiding this comment.
I think that Util is not a very suitable name for a class. Could you please choose something more descriptive?
There was a problem hiding this comment.
Thank you. I use Composer to replace Util. Do you think that's ok?
| } | ||
|
|
||
| RedisDatabase::Util::Util(const std::string &file_name) { | ||
| file_.open(file_name, std::ofstream::binary); |
There was a problem hiding this comment.
What happens if a file fails to open? Will the user be informed?
There was a problem hiding this comment.
Thank you.Do you think it's OK to throw an exception?
| uint32_t len32 = htonl(len); | ||
| s = saveRaw(&len32, 4); | ||
| } | ||
| return s; |
There was a problem hiding this comment.
What happens if the length exceeds uint32?
There was a problem hiding this comment.
According to the kvrocks metadata size(4byte) design, this situation does not exist.
There was a problem hiding this comment.
I think you can use uint32_t if it will not happen.
| */ | ||
|
|
||
| #define RDB_VERSION 6 | ||
| #define SELECT_DB 0 |
There was a problem hiding this comment.
I prefer a more concrete name here.
There was a problem hiding this comment.
May I ask for a suggested name?
|
@ColinChamber Can you help to add more background information? |
Signed-off-by: tison <wander4096@gmail.com>
| class CommandSave: public Commander { | ||
| public: | ||
| Status Execute(Server *svr, Connection *conn, std::string *output) override { | ||
| Redis::RedisDatabase RDB(svr->storage_, conn->GetNamespace()); |
There was a problem hiding this comment.
I'd prefer rdb instead of RDB variable name
| #include "redis_db.h" | ||
|
|
||
| namespace Redis { | ||
| class RedisDatabase : public Database { |
There was a problem hiding this comment.
@ColinChamber I think you can rename RedisDatabase to something like DumpCreator and move all the functionality from Composer to DumpCreator. Thus, you won't have RedisDatabase with just 1 method and inside it one private class and such long things like RedisDatabase::Composer::MethodName(). What do you think?
There was a problem hiding this comment.
Is the class name DumpCreator ok if the parser is added in the future?
There was a problem hiding this comment.
Do you mean the parser for importing the dump?
There was a problem hiding this comment.
Parse the Redis dump file. I know few people need it, but if someone wants to achieve it.
There was a problem hiding this comment.
So you think about the RedisDatabase class and its inner classes, for example, DumpCreator and DumpLoader/DumpImporter/etc. ?
| const int RDB_32BITLEN = 0x80; | ||
|
|
||
| enum RDBOpCode: unsigned char { | ||
| RDB_OPCODE_EXPIRETIME = 253, /* Expire time in seconds. */ |
There was a problem hiding this comment.
@ColinChamber Additionally you can have a look at this commit (1dcf225) and pay attention to const and enum naming to make relevant changes in your PR.
tanruixiang
left a comment
tanruixiang
left a comment
There was a problem hiding this comment.
Hi. I found a simple package name typo.
| * under the License. | ||
| */ | ||
|
|
||
| package integration |
There was a problem hiding this comment.
| package integration | |
| package save |
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
I'd prefer you create an issue or at least update the PR description about why you made this change. Otherwise, we review the implementation details but lose the aim of doing all this stuff.
There is an issue #1001, although it has not been linked to this PR. |
|
@PragmaTwice Thank you! I'll check it. |
|
There's a little tricky to dump RDB in Kvrocks. My current test for this PR is far from enough. I'm sorry that I should close it first. Maybe I will resubmit after I understand every detail. Thanks to every reviewer. |
6 participants
This refers to #1001.