Add I/O tailer for index meta check #240

cqy123456 · 2023-11-30T14:54:01Z

No description provided.

sre-ci-robot · 2023-11-30T14:54:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cqy123456

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cqy123456]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mergify · 2023-11-30T14:54:39Z

@cqy123456 🔍 Important: PR Classification Needed!

For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:

If you're fixing a bug, label it as kind/bug.
For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
Adjusting APIs or changing functionality? Go with kind/feature.

For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”.

Thanks for your efforts and contribution to the community!.

alexanderguzhva · 2023-11-30T15:10:40Z

this changes the index file format, right?

foxspy · 2023-12-01T07:45:13Z

src/io/tailer.h

+        uint64_t bin_size;
+        IndexVersion version;
+        uint32_t checksum;
+        char index_name[MAX_INDEX_NAME_SIZE + 1];


reserve some buffer for future extension, or else adding new meta could be difficult

Tailer has 512bytes，it can add more meta info within 512bytes. Maybe we use larger size ?

foxspy · 2023-12-01T07:53:54Z

src/io/memory_io.h

@@ -100,6 +102,9 @@ struct MemoryIOWriter : public faiss::IOWriter {
    tellg() const {
        return rp_;
    }
+
+    void


Add same interface for FileIOWriter, to ensure the binary info is independent to IOWriter.

I will remove addtailer and valid check in IOReader/Writer, and add AddTailerForMultiFiles/AddTailerForMemoryIO functions.

foxspy · 2023-12-01T08:11:49Z

src/index/ivf/ivf.cc

@@ -846,6 +847,7 @@ IvfIndexNode<faiss::IndexIVFFlat>::Serialize(BinarySet& binset) const {
            faiss::write_index(index_.get(), &writer);
            LOG_KNOWHERE_INFO_ << "write IVF_FLAT, file size " << writer.tellg();
        }
+        writer.addtailer(Type(), this->version_);


Can move the addtailer to index.cc? Just a suggestion, maybe different to index ser/des with file.

zhengbuqian · 2023-12-07T06:56:42Z

src/io/memory_io.h

@@ -71,6 +70,7 @@ getSwappedBytes(char C) {

 #endif

+// MemoryIOwriter and MemoryIOreader it not thread safe


typo "are not"

zhengbuqian · 2023-12-07T07:05:03Z

src/io/trailer.h

+    void
+    SetIndexName(std::string index_name) {
+        if (index_name.size() > MAX_INDEX_NAME_SIZE) {
+            LOG_KNOWHERE_ERROR_ << "the size of index name larger than " << MAX_INDEX_NAME_SIZE;


we should not just silently ignore the error. we should throw exception or return false so the caller of this method is aware of the error and can handle it.

zhengbuqian · 2023-12-07T07:12:41Z

src/io/trailer.h

+        IndexVersion version;
+        uint32_t checksum;
+        char index_name[MAX_INDEX_NAME_SIZE + 1];
+    } meta;


consider making members private.

zhengbuqian · 2023-12-07T07:16:33Z

src/io/trailer.cc

+uint32_t
+GetFilesCheckSum(std::vector<std::string> files) {
+    uint32_t checksum = 0;
+    auto buffer = std::shared_ptr<uint8_t[]>(new uint8_t[kBlockSize]);


use unique ptr as it is never shared.

zhengbuqian · 2023-12-07T07:31:18Z

src/io/trailer.h

+    GetIndexName() {
+        return std::string(meta.index_name);
+    }
+    int32_t


why not use type alias IndexVersion?

zhengbuqian · 2023-12-07T07:32:32Z

src/io/trailer.cc

+    reader.read(trailer_ptr.get(), KNOWHERE_TRAILER_SIZE);
+    reader.seekg(pre_rp);
+
+    if (!trailer_ptr->TrailerValidCheck()) {


what if this file has no trailer but these 4 bytes happens to be the same as TRAILER_VALID_FLAG?

is it possible to use version number to decide whether a index file should include trailer?

I added flags at the head and tail of the trailer for checking.

zhengbuqian · 2023-12-07T07:37:09Z

src/io/trailer.cc

+    }
+    auto trailer_ptr = std::make_unique<Trailer>();
+    reader.read((char*)trailer_ptr.get()->bytes, KNOWHERE_TRAILER_SIZE);
+    if (!trailer_ptr->TrailerValidCheck()) {


avoid duplicating those checks in CheckTrailerForFiles and CheckTrailerForMemoryIO. trailer class can have method validate(name, checksum) to do those checks.

zhengbuqian · 2023-12-07T07:38:56Z

src/io/trailer.h

+AddTrailerForFiles(const std::vector<std::string>& files, const std::string& trailer_file, const std::string& name,
+                   const Version& version);
+bool
+CheckTrailerForMemoryIO(MemoryIOReader& reader, const std::string& name);


I see no usage of CheckTrailer in deserialize methods of memory indexes?

next pr. This pr verifies old knowhere code also can deserialize with the new binaryset.

zhengbuqian · 2023-12-07T07:45:44Z

src/io/trailer.cc

+AddTrailerForFiles(const std::vector<std::string>& files, const std::string& trailer_file, const std::string& name,
+                   const Version& version) {
+    auto trailer_ptr = std::make_unique<Trailer>();
+    trailer_ptr->SetCheckSum(GetFilesCheckSum(files));


trailer for files is missing binary size. I prefer to make them consistent:

either remove binary size from single binary index and rely solely on checksum for correctness, which should be sufficient?

or add binary size sum of all files for multiple files index

binary size removed.

zhengbuqian · 2023-12-12T07:34:12Z

src/io/trailer.h

+#define TRAILER_OFFSET(size) size - KNOWHERE_TRAILER_SIZE
+
+namespace knowhere {
+struct Meta {


name Meta is too big/wide to put in namespace knowhere. I suggest to hide struct Meta inside the Trailer union, as the outside world doesn't need to know its existence. Also add placeholder for the end flag as well. maybe:

union Trailer { public: ... private: uint8_t bytes[KNOWHERE_TRAILER_SIZE]; struct Meta { uint32_t beg_flag_placeholder; struct Data { uint32_t placeholder; IndexVersion version; uint32_t checksum; char index_name[MAX_INDEX_NAME_SIZE + 1]; } data; char unused[512 - 2 * sizeof(uint32_t) - sizeof(Data)]; uint32_t end_flag_placeholder; } meta; }

I was wrong, my proposed solution could result in struct member alignment error if we add 8 bytes members

zhengbuqian · 2023-12-12T08:04:52Z

src/io/trailer.h

+    bool
+    TrailerValidCheck() {
+        FLAG_TYPE* buf = (FLAG_TYPE*)bytes;
+        return buf[BEG_FLAG_OFFEST] == FLAG_MAGIC_NUMBER && buf[END_FLAG_OFFEST] == FLAG_MAGIC_NUMBER;


you can check on meta members directly

meta.placeholder == FLAG_MAGIC_NUMBER

zhengbuqian · 2023-12-12T08:21:14Z

src/io/trailer.cc

+
+    reader.seekg(0, std::ios::beg);
+    if (fsize != KNOWHERE_TRAILER_SIZE) {
+        LOG_KNOWHERE_ERROR_ << "Trailer size (" << fsize << ")not correct.";


should "trailer file exists but has incorrect size" indicate error instead of success?

chatted offline, we should consider add a constant max_version_without_trailer to the knowhere versioning system, all indexes with a greater version should include trailer, or else error.

zhengbuqian · 2023-12-14T04:12:04Z

src/io/trailer.cc

+}
+
+knowhere::Status
+TrailerCheck(const knowhere::TrailerPtr& trailer_ptr, const std::string& name, const uint32_t checksum) {


consider making this a method of the Trailer class

Signed-off-by: cqy123456 <[email protected]>

sre-ci-robot requested review from foxspy and zhengbuqian November 30, 2023 14:54

sre-ci-robot added approved size/L labels Nov 30, 2023

mergify bot added the dco-passed label Nov 30, 2023

mergify bot added the do-not-merge/missing-related-issue label Nov 30, 2023

mergify bot added the ci-passed label Nov 30, 2023

foxspy reviewed Dec 1, 2023

View reviewed changes

mergify bot removed the ci-passed label Dec 4, 2023

cqy123456 force-pushed the tailer branch 3 times, most recently from 45e6f84 to 2b87809 Compare December 6, 2023 12:38

mergify bot added the ci-passed label Dec 6, 2023

zhengbuqian reviewed Dec 7, 2023

View reviewed changes

cqy123456 force-pushed the tailer branch from 2b87809 to 48a16f1 Compare December 12, 2023 01:57

mergify bot removed the ci-passed label Dec 12, 2023

cqy123456 force-pushed the tailer branch from 48a16f1 to a135c73 Compare December 12, 2023 02:33

mergify bot added the ci-passed label Dec 12, 2023

cqy123456 force-pushed the tailer branch from a135c73 to 865bdd0 Compare December 12, 2023 03:49

mergify bot removed the ci-passed label Dec 12, 2023

cqy123456 force-pushed the tailer branch from 865bdd0 to 1cdd32f Compare December 12, 2023 05:06

mergify bot added the ci-passed label Dec 12, 2023

cqy123456 force-pushed the tailer branch from 1cdd32f to 2d65386 Compare December 12, 2023 06:25

mergify bot removed the ci-passed label Dec 12, 2023

zhengbuqian reviewed Dec 12, 2023

View reviewed changes

zhengbuqian reviewed Dec 14, 2023

View reviewed changes

cqy123456 force-pushed the tailer branch from 2d65386 to 6dca7ab Compare December 14, 2023 09:47

add io trailer for knowhere

32871be

Signed-off-by: cqy123456 <[email protected]>

cqy123456 force-pushed the tailer branch from 6dca7ab to 32871be Compare December 14, 2023 09:50

mergify bot added the ci-passed label Dec 14, 2023

github-actions bot added the stale label Mar 5, 2024

mergify bot added needs-dco and removed dco-passed ci-passed labels Mar 6, 2024

github-actions bot removed the stale label Mar 7, 2024

github-actions bot added the stale label Apr 6, 2024

github-actions bot closed this Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add I/O tailer for index meta check #240

Add I/O tailer for index meta check #240

cqy123456 commented Nov 30, 2023

sre-ci-robot commented Nov 30, 2023

mergify bot commented Nov 30, 2023

alexanderguzhva commented Nov 30, 2023

foxspy Dec 1, 2023

cqy123456 Dec 5, 2023

foxspy Dec 1, 2023

cqy123456 Dec 5, 2023

foxspy Dec 1, 2023

zhengbuqian Dec 7, 2023 •

edited

Loading

cqy123456 Dec 12, 2023

zhengbuqian Dec 7, 2023

cqy123456 Dec 12, 2023

zhengbuqian Dec 7, 2023

cqy123456 Dec 12, 2023

zhengbuqian Dec 7, 2023

cqy123456 Dec 12, 2023

zhengbuqian Dec 7, 2023

cqy123456 Dec 12, 2023

zhengbuqian Dec 7, 2023

cqy123456 Dec 12, 2023

zhengbuqian Dec 7, 2023

cqy123456 Dec 12, 2023 •

edited

Loading

zhengbuqian Dec 7, 2023

cqy123456 Dec 12, 2023

zhengbuqian Dec 7, 2023

cqy123456 Dec 12, 2023

zhengbuqian Dec 12, 2023

zhengbuqian Dec 12, 2023

zhengbuqian Dec 12, 2023

zhengbuqian Dec 12, 2023

zhengbuqian Dec 12, 2023

zhengbuqian Dec 14, 2023

		@@ -71,6 +70,7 @@ getSwappedBytes(char C) {

		#endif

		// MemoryIOwriter and MemoryIOreader it not thread safe

Add I/O tailer for index meta check #240

Add I/O tailer for index meta check #240

Conversation

cqy123456 commented Nov 30, 2023

sre-ci-robot commented Nov 30, 2023

mergify bot commented Nov 30, 2023

alexanderguzhva commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbuqian Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqy123456 Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbuqian Dec 7, 2023 •

edited

Loading

cqy123456 Dec 12, 2023 •

edited

Loading