[fix] curvefs: mds: createfs error #2791
Conversation
201341
force-pushed
the
swj/fix-createfs
branch
2 times, most recently
from
b2916c4 to
a7109a7
Compare
|
cicheck |
|
@Cyber-SiKu PTAL |
201341
force-pushed
the
swj/fix-createfs
branch
from
a7109a7 to
2b103f7
Compare
|
cicheck |
|
@Cyber-SiKu PTAL |
201341
force-pushed
the
swj/fix-createfs
branch
4 times, most recently
from
af61bbe to
ac73df0
Compare
|
cicheck |
1 similar comment
|
cicheck |
curvefs/src/mds/fs_manager.cpp
Outdated
| if (lhs.has_objectprefix() && !rhs.has_objectprefix() && | ||
| lhs.objectprefix() != 0) { | ||
| return false; | ||
| } | ||
| if (rhs.has_objectprefix() && !lhs.has_objectprefix() && | ||
| rhs.objectprefix() != 0) { | ||
| return false; | ||
| } | ||
|
|
||
| return google::protobuf::util::MessageDifferencer::Equals(lhs, rhs); |
There was a problem hiding this comment.
| if (lhs.has_objectprefix() && !rhs.has_objectprefix() && | |
| lhs.objectprefix() != 0) { | |
| return false; | |
| } | |
| if (rhs.has_objectprefix() && !lhs.has_objectprefix() && | |
| rhs.objectprefix() != 0) { | |
| return false; | |
| } | |
| return google::protobuf::util::MessageDifferencer::Equals(lhs, rhs); | |
| if (lhs.has_prefix() && rhs.has_prefix() && (lhs.prefix() != rhs.prefix())) { | |
| return false; | |
| } | |
| lhs.clear_prefix(); | |
| rhs.clear_prefix(); | |
| return Equals(lhs, rhs); |
Are they equivalent? If so, this might look clearer. BTW, pls add corresponding tests.
There was a problem hiding this comment.
No, it's different. This change is mainly for compatibility with old clients which don't have object_prefix attribute. For test, I will add it later.
There was a problem hiding this comment.
No, it's different. This change is mainly for compatibility with old clients which don't have object_prefix attribute. For test, I will add it later.
The problem is clear, and, as my understanding, we just need to ensure if both have prefix, then they must be same, otherwise, we can ignore this field.
There was a problem hiding this comment.
It can't cover the condition that the old client which doesn't have prefix, and the new client has the prefix which is 1 .
201341
force-pushed
the
swj/fix-createfs
branch
2 times, most recently
from
17b3b11 to
06391b8
Compare
|
cicheck |
4 similar comments
|
cicheck |
|
cicheck |
|
cicheck |
|
cicheck |
|
@wu-hanqing PTAL |
curvefs/src/mds/fs_manager.cpp
Outdated
| auto s3InfoComparator = [](common::S3Info lhs, common::S3Info rhs) { | ||
| // for compatible with old clients |
There was a problem hiding this comment.
Please add more comments to explain scenarios that require compatibility, and renaming parameters because it is impossible to clearly distinguish which is the MDS side and which is the fs the current client is preparing to create.
There was a problem hiding this comment.
The parameters' meaning depends on the caller, If changing the parameters, the caller must pass the parameter in the correct order. Is there any need to make changes?
There was a problem hiding this comment.
The parameters' meaning depends on the caller, If changing the parameters, the caller must pass the parameter in the correct order. Is there any need to make changes?
The parameters of a function are part of the calling convention and should not depend on the caller. The caller needs to pass in the parameters according to the parameters of the function as specified.
201341
force-pushed
the
swj/fix-createfs
branch
from
06391b8 to
da51079
Compare
201341
force-pushed
the
swj/fix-createfs
branch
from
da51079 to
97a4b76
Compare
|
cicheck |
Signed-off-by: swj <[email protected]>
201341
force-pushed
the
swj/fix-createfs
branch
from
97a4b76 to
9541d12
Compare
|
cicheck |
1 similar comment
|
cicheck |
What problem does this PR solve?
Issue Number: #2779
Problem Summary:
What is changed and how it works?
What's Changed:
How it Works:
Side effects(Breaking backward compatibility? Performance regression?):
Check List