sodonnel commented on pull request #3231:
URL: https://github.com/apache/ozone/pull/3231#issuecomment-1077567323
Thanks for fixing this. The change LGTM, but I have a general question about
the need for this "client version" in this case.
I see in the server side code, the logic is like:
```
for (Port port : ports) {
if (handlesUnknownPorts || Name.V0_PORTS.contains(port.getName())) {
builder.addPorts(port.toProto());
} else {
if (LOG.isDebugEnabled()) {
LOG.debug("Skip adding {} port {} to proto message for client v{}",
port.getName(), port.getValue(), clientVersion);
}
}
}
```
So we skip emitting various ports of the client is too old to understand
them. However in the proto, the port is passed as a string. So proto supports
us sending any port name. The general philosophy is that proto messages are
backward compatible as we never remove old files and only add new fields, as
the client will not attempt to access fields it does not know how to translate.
In this case, I guess it would have been better if the proto message looked
like:
```
optional Port ratisPort = 4;
optional Port replicationPort = 5;
...
```
Ie a distinct set of values rather than a repeated Ports ield. That would
have avoided this whole problem. We could even have deprecated the original
ports field and added the above instead and ensure the original port list was
in the deprecated field. The forward compatibility would have been maintained
without passing the client version string around, right? I do admit this is not
as nice to work with as a list, as if you want to display the list of ports,
you can no-longer just loop over them.
Even without this, could we not have solved this problem when de-serializing
the proto object by simply skipping any ports we don't know about on the
client, rather than omitting them on the server (again avoiding the need to
pass the client version around)? I suspect this original change came about due
to an unknown enum error on de-serialize.
Note I am not suggesting we change this now - I am trying to understand why
things are this way, as I feel its easy to miss these things as I did when I
made the original change, and it looks like there could have been better ways
to solve this that look simpler to me.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
