Skip to content

Conversation

@adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Jan 22, 2024

What changes were proposed in this pull request?

Revert HDDS-9426 (#5579 and addendum), because it introduced incompatible proto changes (#6044 (comment)). These were not caught because backwards compatibility check was still comparing to Ozone 1.3.0's proto definition. But HDDS-9426 is not present in 1.4.0, it was implemented during the release process. So we need it to be compatible with 1.4.0 proto definitions.

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

How was this patch tested?

Verified backwards compatibility check does not report any errors with proto.lock changes from #6044 after this revert.

$ mvn -DskipTests clean verify
...
[INFO] --- proto-backwards-compatibility:1.0.7:backwards-compatibility-check (default) @ ozone-interface-client ---
[INFO] protolock cmd line: hadoop-ozone/interface-client/target/protolock-bin/protolock status --lockdir=hadoop-ozone/interface-client/target/classes --protoroot=hadoop-ozone/interface-client/target/classes
[INFO] protolock cmd line: hadoop-ozone/interface-client/target/protolock-bin/protolock commit --lockdir=hadoop-ozone/interface-client/target/classes --protoroot=hadoop-ozone/interface-client/target/classes
[INFO] Backwards compatibility check passed.

@swamirishi
Copy link
Contributor

@aswinshakil Can we make the changes to make the changes protobuf compatible instead of reverting the commit all together. I believe it shouldn't be that big a change.

@adoroszlai
Copy link
Contributor Author

@swamirishi CI cannot verify compatibility until proto.lock changes for 1.4.0 are in. But it cannot be committed until this incompatible change is there. So the simplest way is:

  1. revert
  2. commit proto.lock
  3. re-create PR for this change

@swamirishi
Copy link
Contributor

@swamirishi CI cannot verify compatibility until proto.lock changes for 1.4.0 are in. But it cannot be committed until this incompatible change is there. So the simplest way is:

  1. revert
  2. commit proto.lock
  3. re-create PR for this change

Makes sense

Copy link
Contributor

@swamirishi swamirishi left a comment

Choose a reason for hiding this comment

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

Thanks @adoroszlai for looking into the change. Overall I am fine with the revert plan.

@adoroszlai
Copy link
Contributor Author

Thanks @swamirishi, I'll wait a bit for others to check.

If anyone wants to merge this, please add the following in the "extended description":

Reason for revert: incompatible proto changes

This reverts commit 05284942fa5ec8026de0618f61af9b7cd758a90b.
This reverts commit d969689e83241e2d5bec2c878324ee1ce84c0fe4.

@jojochuang jojochuang removed the request for review from aswinshakil January 22, 2024 17:15
Copy link
Member

@aswinshakil aswinshakil left a comment

Choose a reason for hiding this comment

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

Thanks for finding this and reverting @adoroszlai. I was not aware of this incompatibility.

As suggested, we can revert #5579 and merge #6044. I'll raise a PR for this change once both are merged.

@adoroszlai adoroszlai merged commit 2bf7135 into apache:master Jan 22, 2024
@adoroszlai adoroszlai deleted the revert-HDDS-9426 branch January 22, 2024 18:01
@adoroszlai
Copy link
Contributor Author

Thanks @aswinshakil, @swamirishi for the review.

I was not aware of this incompatibility.

No problem, that's why the check exists. For future releases we should update proto.lock sooner, maybe right after cutting the release branch.

Tejaskriya pushed a commit to Tejaskriya/ozone that referenced this pull request Jan 24, 2024
…'s deleted directories. (apache#5579)" (apache#6051)

Reason for revert: incompatible proto changes

This reverts commit 0528494.
This reverts commit d969689.
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.

3 participants