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

curvebs: support poolset #1988

Merged
merged 4 commits into from
Jun 8, 2023
Merged

Conversation

jolly-sy
Copy link
Contributor

What problem does this PR solve?

Issue Number: #xxx

Problem Summary:

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

@jolly-sy
Copy link
Contributor Author

recheck

2 similar comments
@jolly-sy
Copy link
Contributor Author

recheck

@jolly-sy
Copy link
Contributor Author

recheck

@jolly-sy jolly-sy force-pushed the fixPoolsetConflic branch 2 times, most recently from a46f82b to 3c50c00 Compare October 19, 2022 09:42
@jolly-sy
Copy link
Contributor Author

recheck

@jolly-sy jolly-sy force-pushed the fixPoolsetConflic branch 2 times, most recently from 789c384 to 5d0e610 Compare October 19, 2022 12:01
@jolly-sy
Copy link
Contributor Author

recheck

2 similar comments
@jolly-sy
Copy link
Contributor Author

recheck

@jolly-sy
Copy link
Contributor Author

recheck

@jolly-sy jolly-sy force-pushed the fixPoolsetConflic branch 2 times, most recently from 48932f1 to 670ac54 Compare October 20, 2022 02:44
@jolly-sy
Copy link
Contributor Author

recheck

1 similar comment
@jolly-sy
Copy link
Contributor Author

recheck

Copy link
Contributor

@wu-hanqing wu-hanqing left a comment

Choose a reason for hiding this comment

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

BTW, delete src/mds/nameserver2/curveadm.

And, write a document for this pr, explain how to upgrade old cluster

LOG(ERROR) << "No poolsets in cluster map";
return -1;
}
for (const auto poolset : clusterMap_[kPoolsets]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

using const reference to avoid unnecessary copying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using const reference to avoid unnecessary copying.

fix

tools/curvefsTool.cpp Outdated Show resolved Hide resolved
} else if (s == "nvme") {
poolsetData.type = PoolsetType::NVME;
} else {
poolsetData.type = PoolsetType::SSD;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to report an error in this case instead of giving it a default type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's better to report an error in this case instead of giving it a default type.

fix

Comment on lines 408 to 404
if (!server[kPhysicalPool].isString()) {
LOG(ERROR) << "server physicalpool must be string";
return -1;
}
serverData.physicalPoolName = server[kPhysicalPool].asString();
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems these code are identical with L402-L406.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems these code are identical with L402-L406.

fix

poolsetData.type = PoolsetType::SSD;
}

poolsetDatas.emplace_back(poolsetData);
Copy link
Contributor

Choose a reason for hiding this comment

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

poolsetDatas.emplace_back(std::move(poolsetData))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

poolsetDatas.emplace_back(std::move(poolsetData))

fix

@@ -213,6 +233,7 @@ bool PhysicalPool::ParseFromString(const std::string &value) {
id_ = data.physicalpoolid();
name_ = data.physicalpoolname();
desc_ = data.desc();
poolsetId_ = data.poolsetid();
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, you should check whether data has poolset id or not, if it don't have it, poolsetId_ should set to UNINTIALIZE_ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix
if(data.has_poolsetid()){
poolsetId_ = data.poolsetid();
}

@@ -97,6 +98,7 @@ class DefaultIdGenerator : public TopologyIdGenerator {
std::atomic<T> idMax_;
};

IdGenerator<PoolsetIdType> poolsetIdGentor_;
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems poolsetIdGentor_ doesn't inited

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems poolsetIdGentor_ doesn't inited

fix, poolsetIdGentor_ is inited

Copy link
Contributor

@ilixiaocui ilixiaocui Dec 22, 2022

Choose a reason for hiding this comment

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

poolsetIdGentor_ init in class TopologyIdGenerator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where does poolsetIdGentor_ inited?
in class TopologyIdGenerator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where does poolsetIdGentor_ inited?

in class TopologyIdGenerator

}
}

// int TopologyImpl::RemovePhysicalPoolNotInPoolset(PoolIdType id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove unused code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove unused code

fix, the unused code were removed

auto ifok = chunkSegAllocator_->AllocateChunkSegment(
fileInfo.filetype(), fileInfo.segmentsize(),
fileInfo.chunksize(), offset, segment);
fileInfo.chunksize(), poolset.GetType(),
Copy link
Contributor

Choose a reason for hiding this comment

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

noop, segment should be allocated by poolset name instead of type, because we can have more than one poolsets with same type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noop, segment should be allocated by poolset name instead of type, because we can have more than one poolsets with same type.

fix, parameter has been changed to poolsetName

@@ -1251,9 +1258,15 @@ StatusCode CurveFS::GetOrAllocateSegment(const std::string & filename,
return StatusCode::kSegmentNotAllocated;
} else {
// TODO(hzsunjianliang): check the user and define the logical pool
if (!fileInfo.has_poolsetname()) {
fileInfo.set_poolsetname("ssdPoolset1");
Copy link
Contributor

Choose a reason for hiding this comment

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

it's no a good idea, a proper way is adding a config item in mds configuration to store the default poolsetname for allocating segment for those files that don't have a poolsetname in fileinfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's no a good idea, a proper way is adding a config item in mds configuration to store the default poolsetname for allocating segment for those files that don't have a poolsetname in fileinfo.

fix, defalut poolsetName has been set in mds.conf

@jolly-sy jolly-sy force-pushed the fixPoolsetConflic branch 2 times, most recently from 345b4cf to 44cdbc2 Compare November 21, 2022 12:36
@jolly-sy jolly-sy force-pushed the fixPoolsetConflic branch 7 times, most recently from 7ff7e02 to b339c17 Compare December 13, 2022 08:31
@jolly-sy
Copy link
Contributor Author

cicheck

@wu-hanqing
Copy link
Contributor

cicheck

@wu-hanqing
Copy link
Contributor

cicheck

@wu-hanqing
Copy link
Contributor

cicheck

*
* @return: success return 0, fail return less than 0
*/
int Create2(const char* filename,
Copy link
Member

Choose a reason for hiding this comment

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

why remove Create2?

@@ -93,6 +93,7 @@ message FileInfo {

optional FileThrottleParams throttleParams = 17;
optional uint64 epoch = 18;
optional string poolset = 19;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to use poolsetid instead?

wu-hanqing and others added 3 commits June 3, 2023 19:11
Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>
Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>
Co-authored-by: jolly-sy <757050468@qq.com>
Co-authored-by: Hanqing Wu <wuhanqing@corp.netease.com>

Signed-off-by: jolly-sy <757050468@qq.com>
Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>
@wu-hanqing
Copy link
Contributor

cicheck

1 similar comment
@wu-hanqing
Copy link
Contributor

cicheck

@wu-hanqing
Copy link
Contributor

cicheck

…directories

Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>
@wu-hanqing
Copy link
Contributor

cicheck

@ilixiaocui
Copy link
Contributor

Please add a poolset-related ReadMe in the appropriate section, for exmaple: how to use

@wu-hanqing
Copy link
Contributor

Please add a poolset-related ReadMe in the appropriate section, for exmaple: how to use

I will update curveadm wiki later.

@wu-hanqing wu-hanqing merged commit 191a001 into opencurve:master Jun 8, 2023
3 checks passed
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.

None yet

5 participants