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

HDDS-3621. Separate client/server/admin proto files of HDDS to separated subprojects #949

Merged
merged 4 commits into from Jun 5, 2020

Conversation

elek
Copy link
Member

@elek elek commented May 20, 2020

What changes were proposed in this pull request?

As described in the parent Jira admin/client/server protocols need different compatibility guarantees. It's better to separate them to separated maven project to make it easy to follow the changes.

I propose to create 3 new projects

  • hadoop-hdds/interface-client
  • hadoop-hdds/interface-server
  • hadoop-hdds/interface-admin`

I called it -interface instead of -proto because we can include some basic Java classes not just proto files (for example utilities to serialize/deserialize proto files)

This first patch will include only the move without refactoring any RPC. While the new proto file names represent the proposed naming convention (Datanode/Scm + Server/Client/Admin + additional postfix) the name of the generated classes and the name of the RPC interfaces (Client/Server translator) not yet renamed to make the patch small.

Also: some methods should be moved between admin/server but it can be done in separated issue to make it easier to follow the change.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3621

How was this patch tested?

This patch includes only moves no refactor. Only the names of the proto files are changed no logic.

Tested with full CI on my fork.

Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

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

LGTM overall. Just a few comments. It seems difficult to add inline comments to rename changes. I just added all comments here.

  1. The first commit is shown as "HDDS-3519. Finalize network and storage protocol of Ozone". not HDDS-3621.
  2. NIT: Can we remove the Server from those SCM related .proto file?
  3. Can we rename SCMServerProtocol.proto as SCMServerBlock.proto?
  4. hadoop-hdds-interface-client/src/main/proto misses the proto.lock file added by HDDS-3595.

@elek
Copy link
Member Author

elek commented May 27, 2020

Thanks thre review @xiaoyuyao

The first commit is shown as "HDDS-3519. Finalize network and storage protocol of Ozone". not HDDS-3621.

Ups, yes, that's the parent issue. I need a force push to fix it, but it can be more safe. Will do.

Can we rename SCMServerProtocol.proto as SCMServerBlock.proto?

I am not sure about this. It's a question of the granularity. The idea was to put ALL the intra-server related to one service / proto file of each of the servers. If something is used between two servers it should be part of a dedicated service (but only one service, we don't need separated block / container location services if they are used in the same way).

SCM is very specific, I felt that the Datanode HB protocol (and security) is so important and big, that it can be reasonable to keep it as is (a separated service). But as it's an exception I tried to follow the original convention (server + type + identifier):

  • server=scm, type=server, identifier=heartbeat (ScmServerDatanodeHeartbeatProtocol.proto)
  • server=scm, type=server, identifier=security (ScmServerSecurityProtocol.proto)
  • server=scm, type=server, identifier=all the remaining (ScmServerProtocol.proto)

So this was the reason to use ScmServerProtocol.proto instead of ScmServerBlockProtocol.proto. Because I assume that all the server related protocols should be moved to here (actually some part of the ScmAdminProtocol.proto should also be moved to here, which is used from other server instead of the tools).

I can be convinced to use a different scheme, but only after I shared my thinking here ;-)

NIT: Can we remove the Server from those SCM related .proto file?

Here I am not sure about the suggestion. Do you suggest it to remove Server from the name of the files?

I can do it (as the project name is already a classifier) but I think it's less confusing to use exactly the same name everwhere. The service of ScmServerSecurityProtocol supposed to us ScmServerSecurityProtocolClientSideTranslator and ScmServerSecurity.... interfaces, and we need the Server there IMHO. (Unless we start to use better java package names)

hadoop-hdds-interface-client/src/main/proto misses the proto.lock file added by HDDS-3595.

OMG, that's the most important part. I fixed it.

@xiaoyuyao
Copy link
Contributor

xiaoyuyao commented Jun 1, 2020

The idea was to put ALL the intra-server related to one service / proto file of each of the servers. If something is used between two servers it should be part of a dedicated service (but only one service, we don't need separated block / container location services if they are used in the same way)

That sounds good to me. The original reason of separating block and container is for cBlock which only needs container service but not block service.

I can do it (as the project name is already a classifier) but I think it's less confusing to use exactly the same name everywhere.

Good point. Let's keep the same as-is.

@elek
Copy link
Member Author

elek commented Jun 3, 2020

Thanks the answer, If I understood well, we have a consensus. I will merge it if no objection and do the same on the hadoop-ozone side as a next step.

@elek elek merged commit 7f6f813 into apache:master Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants