Skip to content

Conversation

@symious
Copy link
Contributor

@symious symious commented Jan 21, 2024

What changes were proposed in this pull request?

Add proto.lock files from ozone-1.4 release branch to master/

These files are checked as part of the build to prevent protobuf incompatabilities.

What is the link to the Apache JIRA

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

How was this patch tested?

No tests.

@adoroszlai
Copy link
Contributor

adoroszlai commented Jan 22, 2024

Thanks @symious for the patch.

The compatibility check reports some errors:

[INFO] CONFLICT: "SetSnapshotPropertyRequest" field: "snapshotKey" ID: 1 has an updated name, previously "snapshotProperty" [OmClientProtocol.proto]
[INFO] CONFLICT: "SetSnapshotPropertyRequest" field: "snapshotProperty" has been removed, but is not reserved [OmClientProtocol.proto]
[INFO] CONFLICT: "SnapshotProperty" ID: "1" has been removed, but is not reserved [OmClientProtocol.proto]
[INFO] CONFLICT: "SnapshotProperty" ID: "2" has been removed, but is not reserved [OmClientProtocol.proto]
[INFO] CONFLICT: "SnapshotProperty" ID: "3" has been removed, but is not reserved [OmClientProtocol.proto]
[INFO] CONFLICT: "SnapshotProperty" field: "exclusiveReplicatedSize" has been removed, but is not reserved [OmClientProtocol.proto]
[INFO] CONFLICT: "SnapshotProperty" field: "exclusiveSize" has been removed, but is not reserved [OmClientProtocol.proto]
[INFO] CONFLICT: "SnapshotProperty" field: "snapshotKey" has been removed, but is not reserved [OmClientProtocol.proto]
[INFO] CONFLICT: "SnapshotPurgeRequest" ID: "2" has been removed, but is not reserved [OmClientProtocol.proto]
[INFO] CONFLICT: "SnapshotPurgeRequest" field: "updatedSnapshotDBKey" has been removed, but is not reserved [OmClientProtocol.proto]

caused by changes introduced in #5579:

@@ -1891,15 +1892,16 @@ message SnapshotMoveKeyInfos {
 
 message SnapshotPurgeRequest {
   repeated string snapshotDBKeys = 1;
-  repeated string updatedSnapshotDBKey = 2;
 }
 
 message SetSnapshotPropertyRequest {
-  optional SnapshotProperty snapshotProperty = 1;
+  optional string snapshotKey = 1;
+  optional SnapshotSize snapshotSize = 2;
+  optional bool deepCleanedDeletedDir = 3;
+  optional bool deepCleanedDeletedKey = 4;
 }
 
-message SnapshotProperty {
-  optional string snapshotKey = 1;
+message SnapshotSize {
   optional uint64 exclusiveSize = 2;
   optional uint64 exclusiveReplicatedSize = 3;
 }

If I understand correctly, build for that PR would have failed if the proto.lock files were updated sooner after the branch cut.

I'll submit a PR to revert #5579, then we can revisit this one. BTW, it would be better to recreate this branch (after the revert) in your fork.

@jojochuang
Copy link
Contributor

Need to wait until #5579 is merged.

@adoroszlai
Copy link
Contributor

@ChenSammi @errose28 This is ready for review.

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

Shouldn't there be changes to hadoop-hdds/interface-client/src/main/resources/proto.lock in this change as well? Git shows changes to protos in that module since the last release.

$ git log --pretty=reference ./hadoop-hdds/interface-client/src/main/proto | head -n10 
0e07225dbd (HDDS-9807. Consider volume committed space when checking if datanode can host new container (#5721), 2023-12-20)
4788798724 (HDDS-7601. Added new columnFamily: compactionLogTable to store compaction entries (#5303), 2023-09-18)
a83668c0f6 (HDDS-7831. Use symmetric secret key to sign and verify token (#4417), 2023-04-05)
1afb6fa79b (HDDS-7098. Provide a way for admin to identify all unhealthy container replicas (#4443), 2023-05-22)
3f5a80783a (HDDS-7853. Add support for RemoveSCM in SCMRatisServer. (#4358), 2023-03-23)
4a43f343ad (HDDS-8168. Make deadlines inside MoveManager for move commands configurable (#4415), 2023-03-22)
50e57f9af5 (HDDS-7137. Add CLI for Getting the failed deleted block txn (#3691), 2023-03-06)
3d648466ed (HDDS-8032. SCM support reconfigurable dynamically (#4318), 2023-03-02)
469c034423 (HDDS-7687. Support OM transfer Ratis leadership (#4265), 2023-02-14)
2250a481d6 (HDDS-7799. Add container count to datanode usage info (#4209), 2023-02-08)

@symious
Copy link
Contributor Author

symious commented Jan 23, 2024

Shouldn't there be changes to hadoop-hdds/interface-client/src/main/resources/proto.lock in this change as well? Git shows changes to protos in that module since the last release.

There is an error for updating the lock for this directory: protolock update failed for ./hadoop-hdds/interface-client/src/main/resources/../proto

@adoroszlai
Copy link
Contributor

Thanks a lot @errose28 for spotting the missing change. Since this PR just picks the change from Ozone 1.4.0 (9090d52), the release also has incomplete proto.lock.

$ git checkout ozone-1.4.0
$ protolock status --lockdir=hadoop-hdds/interface-client/src/main/resources --protoroot=hadoop-hdds/interface-client/src/main/proto
CONFLICT: "data" was moved into oneof "responseData" [DatanodeClientProtocol.proto]
CONFLICT: "dataBuffers" was moved into oneof "responseData" [DatanodeClientProtocol.proto]

It complains about a change made in 4136d47868e. This is not a recent change, first went into 1.1.0. Same/similar problem for previous releases:

$ git checkout ozone-1.1.0
$ protolock status --lockdir=hadoop-hdds/interface-client/src/main/resources --protoroot=hadoop-hdds/interface-client/src/main/proto
CONFLICT: "data" was moved into oneof "responseData" [DatanodeClientProtocol.proto]

$ git checkout ozone-1.2.0
$ protolock status --lockdir=hadoop-hdds/interface-client/src/main/resources --protoroot=hadoop-hdds/interface-client/src/main/proto
CONFLICT: "data" was moved into oneof "responseData" [DatanodeClientProtocol.proto]
CONFLICT: "dataBuffers" was moved into oneof "responseData" [DatanodeClientProtocol.proto]

...

It turns out to be a new compatibility check added in nilslice/protolock@01572fa recently.

Works fine with the previous release:

$ go install github.com/nilslice/protolock/cmd/[email protected]
$ protolock status --lockdir=hadoop-hdds/interface-client/src/main/resources --protoroot=hadoop-hdds/interface-client/src/main/proto

Similar problem applies to csi.proto, which was last changed in 2019, proto.lock added in 2020.

I suggest:

  1. close this PR
  2. update proto.lock on ozone-1.4 branch (filed HDDS-10187) in two steps (two commits in same PR):
    a. using v0.16.0 to get the missing changes without conflicts
    b. using v0.17.0 and protolock commit --force to persist structures for the new rule
  3. update proto.lock on master in a new PR by taking both 9090d52 and the changes from step 2

@adoroszlai
Copy link
Contributor

Let's create new PR from fork after #6074 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants