feat: search index persistence #1721
Conversation
dranikpg
force-pushed
the
search-persistence
branch
2 times, most recently
from
223e35e
to
a7319dd
Compare
| if (res != facade::RedisParser::Result::OK) { | ||
| LOG(ERROR) << "Bad index definition: " << def; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Script compilation (see above) fails silently, so this should too?
dranikpg
changed the title
src/server/rdb_load.cc
Outdated
| facade::RespVec resp_vec; | ||
| facade::RedisParser parser; | ||
|
|
||
| absl::Span<uint8_t> buffer{reinterpret_cast<uint8_t*>(def.data()), def.size()}; | ||
| auto res = parser.Parse(buffer, &consumed, &resp_vec); | ||
|
|
There was a problem hiding this comment.
Technically we don't need this bulky parser code, we can just split by whitespace (taking into account we build the string like this without any formatting). So RESP would be needed only for quotation marks? 🤔
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
dranikpg
force-pushed
the
search-persistence
branch
from
a7319dd
to
9d23a5b
Compare
| void LoadScriptFromAux(std::string&& value); | ||
| void LoadSearchIndexDefFromAux(std::string&& value); | ||
|
|
||
| private: |
There was a problem hiding this comment.
This private is redundant, you should also get a warning for that. Plz see comment below, and if you don't agree ignore me here :)
There was a problem hiding this comment.
I always do this to group members away from functions. Does it produce a warning? Will look at it
src/server/rdb_save.cc
Outdated
| RETURN_ON_ERR(impl_->SaveAuxFieldStrStr("lua", s)); | ||
| } | ||
|
|
||
| // Save search index definitions only in summary dfs file. |
There was a problem hiding this comment.
Question here, isn't the summary file only part of our extension to the rdb format? Actually this is also a good place to check my mental understanding of this so:
- For plain
rdbcompatible with redis, we produce only one file no matter how many threads we run on. - For our extension, we produce n + 1 files where n is the number of threads + 1 for the summary file.
If the above is true, then shouldn't we also cover the case for plain rdb files ? (assuming here this is 1-1, because this is a module in redis and I am unsure how search works there)
There was a problem hiding this comment.
Oh, will add a comment. For now we decided to be not compatible with rdb
There was a problem hiding this comment.
Yes, your points are correct, I intend to save only inside the dfs summary
src/server/search/doc_index.cc
Outdated
| absl::StrAppend(&out, " PREFIX", " 1 ", base_index.prefix); | ||
|
|
||
| absl::StrAppend(&out, " SCHEMA"); | ||
| for (auto [fname, finfo] : base_index.schema.fields) { |
There was a problem hiding this comment.
deep copy here of std::string && SchemaField. I made a suggestion so you don't have to deal with this :)
|
Nice work 👨🍳 |
Co-authored-by: Kostas Kyrimis <kostaskyrim@gmail.com> Signed-off-by: Vladislav <vladislav.oleshko@gmail.com>
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
| RETURN_ON_ERR(impl_->SaveAuxFieldStrStr("lua", s)); | ||
|
|
||
| if (save_mode_ == SaveMode::RDB) { |
There was a problem hiding this comment.
Nice! So that was a bug!
There was a problem hiding this comment.
No, I just print a warnings now 😄 Unknown aux fields are ignored either way
This PR adds persistence for search indices
Their definitions are stored in plain string form in AUX fields