[HDDS-6365] Relax protolock rule to allow changing field names

• Type: Improvement
• Status: Resolved
• Priority: Major
• Resolution: Won't Do
• Component/s: build
• Labels: pull-request-available
• Assignee: Siyao Meng
• Reporter: Siyao Meng
• Created: 23/Feb/22 05:10
• Updated: 17/Mar/23 09:33
• Resolved: 17/Mar/23 09:33

Description

Motivation

The motivation behind this is that currently in the master branch we use kerberosID as the protobuf message field name while it really means accessId. For example here: https://github.com/apache/ozone/blob/master/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto#L1323

It just so happens to be the case that before Multi-Tenancy (HDDS-4944) is implemented, kerberosID is equivalent to accessId in the context of Ozone S3 access. But with Multi-Tenancy feature this is no longer the case – accessId does not equal to kerberosID any more.

And it could be confusing for other developers to work on S3 / Multi-Tenancy related stuff in the future. e.g. those getKerberosID calls really means getAccessId, and setKerberosID really is setAccessId. Filed HDDS-6339.

Status Quo

Right now proto-backwards-compatibility uses default protolock command line argument, effectively enforcing strict mode for the rules, which doesn't allow changing existing protobuf field names:

https://github.com/apache/ozone/blob/68c5ac5df4fbc0edd7114394112fa696a3dc9229/pom.xml#L1619-L1633

          <groupId>com.salesforce.servicelibs</groupId>
          <artifactId>proto-backwards-compatibility</artifactId>
          <version>${proto-backwards-compatibility.version}</version>
          <configuration>
            <protoSourceRoot>${basedir}/target/classes</protoSourceRoot>
          </configuration>

Changing the field name itself does not break wire compatiblity (field type and field number are still the same), unless that protobuf message is decoded into JSON at some point (which could use the field name as key). Ref: https://stackoverflow.com/a/45431953

Proposal

Add <options>--strict false</options> in pom.xml so that strict mode is off, which as a side affect also turns off two other rules currently enforced according to protolock readme. I'm not sure if protolock provides a way for more granular control of which exact rule to turn off.

No Removing Reserved Fields
Compares the current vs. updated Protolock definitions and will return a list of warnings if any reserved field has been removed.

Note: This rule is not enforced when strict mode is disabled.

No Changing Field Names
Compares the current vs. updated Protolock definitions and will return a list of warnings if any message's previous fields have been renamed.

Note: This rule is not enforced when strict mode is disabled.

No Removing RPCs
Compares the current vs. updated Protolock definitions and will return a list of warnings if any RPCs provided by a Service have been removed.

Note: This rule is not enforced when strict mode is disabled.

Issue Links

• blocks: HDDS-6339 Rename kerberosID to accessId in the code and proto messages
• links to: GitHub Pull Request #3125