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

curvefs/client: fix the data iteration error when rpc retry. #1474

Merged
merged 1 commit into from
May 23, 2022

Conversation

SeanHai
Copy link
Contributor

@SeanHai SeanHai commented May 20, 2022

What problem does this PR solve?

Issue Number: #xxx
#1427
Problem Summary:
The before logic will crash when rcp retry, the iter will forward when retry but this is not expected.
截屏2022-05-20 14 37 07

What is changed and how it works?

What's Changed:

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

  • Relevant documentation/comments is changed or added
  • I acknowledge that all my contributions will be made under the project's license

Comment on lines 503 to 504
std::vector<std::list<uint64_t>> *inodeGroups) {
std::unordered_map<uint32_t, std::list<uint64_t>> groups;
Copy link
Contributor

Choose a reason for hiding this comment

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

for most cases, std::vector is always preferred, std::list/map/set are all node-based containers, they're not cache-friendly.

for std::vector, another tip is if you know the number of elements that it will store, you can call vec.reserve firstly to allocate enough memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 506 to 507
if (ret) {
for (const auto &it : groups) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (!ret) {
    return false;
}

for (...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 551 to 553
for (const auto &id : it) {
request.add_inodeid(id);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

might be a shortcut, *request.mutable_inode() = { it.begin(), it.end() }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

for (const auto &id : it) {
request.add_inodeid(id);
}
if (request.inodeid_size() <= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should you check whether it is empty or not at the beginning? otherwise L537 will be core dumped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

<< ", errmsg = " << MetaStatusCode_Name(ret);
} else if (response.attr_size() > 0 &&
response.has_appliedindex()) {
auto retAttr = response.attr();
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid unnecessary copy, use reference here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 657 to 659
auto retXattr = response.xattr();
for_each(retXattr.begin(), retXattr.end(),
[&](XAttr &a) { xattr->push_back(a); });
Copy link
Contributor

Choose a reason for hiding this comment

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

auto* attrs = response.mutable_xattr();
xattr->insert(xattr->end(),
              std::make_move_iterator(attrs->begin()),
              std::make_move_iterator(attrs->end()));

might be a better way to just move xattrs in response to the destination.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +616 to +623
uint64_t inodeId = *it.begin();
auto task = RPCTask {
metaserverClientMetric_->batchGetXattr.qps.count << 1;
LatencyUpdater updater(
&metaserverClientMetric_->batchGetXattr.latency);
Copy link
Contributor

Choose a reason for hiding this comment

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

the overall process is still serial, can we send requests parallelly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, add a TODO and improve this in later pr.

@SeanHai SeanHai merged commit 01e3745 into opencurve:master May 23, 2022
@SeanHai SeanHai deleted the fix_getxattr branch June 8, 2022 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants