[feat]curvefs: change hostname to optional #2574

201341 · 2023-06-27T12:51:15Z

What problem does this PR solve?

Issue Number: #2493

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

201341 · 2023-06-27T12:52:08Z

cicheck

SeanHai · 2023-07-06T11:14:14Z

curvefs/src/metaserver/register.cpp

-        return -1;
+        LOG(INFO) << "get hostname failed!";
+    } else {
+        req.set_hostname(hostname);


You need change the hostName of MetaServerRegistRequest in topology.proto to optional.

SeanHai · 2023-07-06T11:15:43Z

curvefs/src/tools/create/curvefs_create_topology_tool.cpp

@@ -628,7 +628,9 @@ int CurvefsBuildTopologyTool::CreateServer() {
    for (auto it : serverDatas) {
        ServerRegistRequest request;
        ServerRegistResponse response;
-        request.set_hostname(it.name);
+        if (!it.name.empty()) {
+            request.set_hostname(it.name);


You need change hostName of ServerRegistRequest in topology.topo to optional.

SeanHai · 2023-07-06T11:18:26Z

curvefs/src/mds/topology/topology_manager.cpp

@@ -125,7 +125,12 @@ void TopologyManager::RegistMetaServer(const MetaServerRegistRequest *request,
        }
    }

-    MetaServer metaserver(metaServerId, request->hostname(), token, serverId,
+    std::string hostname;
+    if (request->has_hostname()) {


not need, request->hostname() is empty string if hostname not set.

SeanHai · 2023-07-06T11:22:17Z

You should check all code that uses hostname to see if remove hostname affects it.
For example:

curve/curvefs/src/mds/topology/topology.cpp

Line 1654 in ace571a

std::string TopologyImpl::GetHostNameAndPortById(MetaServerIdType msId) {
You need add some unit test of you modified when not set hostname.

ilixiaocui · 2023-07-06T12:23:40Z

You should check all code that uses hostname to see if remove hostname affects it.
For example:

curve/curvefs/src/mds/topology/topology.cpp

Line 1654 in ace571a

std::string TopologyImpl::GetHostNameAndPortById(MetaServerIdType msId) {

You need add some unit test of you modified when not set hostname.

ditto

ilixiaocui · 2023-07-06T09:55:44Z

curvefs/proto/topology.proto

@@ -79,7 +79,7 @@ message ZoneData {

 message ServerData {
    required uint32 serverId = 1;
-    required string hostName = 2;
+    optional string hostName = 2;


Modify the part of curvebs accordingly.

Yes, I will change the curvebs code in another pr for passing ci.

Signed-off-by: swj <1186093704@qq.com>

201341 · 2023-07-07T13:42:22Z

cicheck

SeanHai · 2023-07-13T02:53:47Z

You should check all code that uses hostname to see if remove hostname affects it.
For example:

curve/curvefs/src/mds/topology/topology.cpp

Line 1654 in ace571a

std::string TopologyImpl::GetHostNameAndPortById(MetaServerIdType msId) {

You need add some unit test of you modified when not set hostname.

The modify of hostname is not only affect register, you need check other place used hostname like above example.

201341 · 2023-07-14T09:36:50Z

You should check all code that uses hostname to see if remove hostname affects it.
For example:

curve/curvefs/src/mds/topology/topology.cpp

Line 1654 in ace571a

std::string TopologyImpl::GetHostNameAndPortById(MetaServerIdType msId) {

You need add some unit test of you modified when not set hostname.

The modify of hostname is not only affect register, you need check other place used hostname like above example.

Yes, But I find no other codes that effect by hostname after searching keyword 'hostname'.

SeanHai · 2023-07-21T06:19:15Z

LGTM!

201341 requested a review from ilixiaocui June 29, 2023 09:21

ilixiaocui requested a review from SeanHai July 6, 2023 09:47

SeanHai reviewed Jul 6, 2023

View reviewed changes

ilixiaocui reviewed Jul 6, 2023

View reviewed changes

[feat]curvefs: change hostname to optional

407b641

Signed-off-by: swj <1186093704@qq.com>

201341 force-pushed the swj/feat-hostname branch from fd76d07 to 407b641 Compare July 7, 2023 13:40

201341 requested review from SeanHai and ilixiaocui July 12, 2023 02:45

SeanHai approved these changes Jul 21, 2023

View reviewed changes

ilixiaocui approved these changes Jul 21, 2023

View reviewed changes

ilixiaocui merged commit c30eff0 into opencurve:master Jul 21, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat]curvefs: change hostname to optional #2574

[feat]curvefs: change hostname to optional #2574

201341 commented Jun 27, 2023 •

edited

201341 commented Jun 27, 2023

SeanHai Jul 6, 2023

201341 Jul 7, 2023

SeanHai Jul 6, 2023

201341 Jul 7, 2023

SeanHai Jul 6, 2023

201341 Jul 7, 2023

SeanHai commented Jul 6, 2023 •

edited

ilixiaocui commented Jul 6, 2023

ilixiaocui Jul 6, 2023

201341 Jul 7, 2023

201341 commented Jul 7, 2023

SeanHai commented Jul 13, 2023

201341 commented Jul 14, 2023 •

edited

SeanHai commented Jul 21, 2023

[feat]curvefs: change hostname to optional #2574

[feat]curvefs: change hostname to optional #2574

Conversation

201341 commented Jun 27, 2023 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

201341 commented Jun 27, 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

Choose a reason for hiding this comment

SeanHai commented Jul 6, 2023 • edited

ilixiaocui commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

201341 commented Jul 7, 2023

SeanHai commented Jul 13, 2023

201341 commented Jul 14, 2023 • edited

SeanHai commented Jul 21, 2023

201341 commented Jun 27, 2023 •

edited

SeanHai commented Jul 6, 2023 •

edited

201341 commented Jul 14, 2023 •

edited