elek commented on pull request #949:
URL: https://github.com/apache/hadoop-ozone/pull/949#issuecomment-634691075
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
