Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix-bug-when-scheam-version-greater-than-256 #4023

Merged
merged 3 commits into from
Mar 17, 2022
Merged

fix-bug-when-scheam-version-greater-than-256 #4023

merged 3 commits into from
Mar 17, 2022

Conversation

sworduo
Copy link
Contributor

@sworduo sworduo commented Mar 14, 2022

What type of PR is this?

  • [☑️] bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

#3993

Description:

How do you solve it?

If the schema version got by doprefix is less than 255, just return. Otherwise, it will iterate all the schema belonging to the queried tag to find the largest one.

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

  • [☑️] Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

@critical27
Copy link
Contributor

Thx for contribution

@critical27 critical27 added the ready-for-testing PR: ready for the CI test label Mar 14, 2022
SchemaVer MetaKeyUtils::getLatestEdgeVersion(kvstore::KVIterator* iter, folly::StringPiece& val) {
SchemaVer maxVer = MetaKeyUtils::parseEdgeVersion(iter->key());
val = iter->val();
if (maxVer >= borderScheVer) {
Copy link
Contributor

@critical27 critical27 Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think maybe you need to iterate no matter what is in maxVer.
For example, if there are 0x7fffffffffffffff schemas (which is std::numeric_limits::max()), when we encode into rocksdb (see MetaKeyUtils::schemaEdgeKey), the bytes are 00 00 00 00 00 00 00 00 , and this will be the first schema since we use rocksdb in byte order, so parseEdgeVersion will return 0....

The root cause is that we encode numbers in little endian, which is not the same order of rocksdb in bytes order.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it is more like a math problem 😢 , I'm really not good at it...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with what critical27 said.
Or distinguish the endianness used.
Or traverse all the keys of the tagId and take the largest version.
The second is easier

Copy link
Contributor

@panda-sheep panda-sheep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

The root cause is a sorting problem caused by little endian mode.
Take the int32_t value 255 or 256 as an example:

int32_t 255(0x000000FF
int32_t 256(0x00000100

The corresponding value in little endian mode

255:  0x FF  00 00 00 
256   0x 00  01 00 00   

The corresponding value in big endian mode:

255:  0x 00  00 00 FF 
256   0x 00  00 01 00   

The little endian mode we use, when sorted in ascending order in rocksdb,
The value 255 comes after only 256, (we would have expected 255 to be before 256)

@@ -560,6 +560,24 @@ SchemaVer MetaKeyUtils::parseEdgeVersion(folly::StringPiece key) {
*reinterpret_cast<const SchemaVer*>(key.begin() + offset);
}

SchemaVer MetaKeyUtils::getLatestEdgeVersion(kvstore::KVIterator* iter, folly::StringPiece& val) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe function name getLatestEdgeVersion needs to be rename.
Here, not only the maximum version is obtained, but the val corresponding to the maximum version is also obtained.

Copy link
Contributor

@panda-sheep panda-sheep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@Sophie-Xie Sophie-Xie merged commit b9d158c into vesoft-inc:master Mar 17, 2022
@sworduo sworduo deleted the fix-schema-greater-than-256 branch March 17, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants