Skip to content

Conversation

@adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Jul 1, 2020

What changes were proposed in this pull request?

Fix compile error in acceptance check on HDDS-2823 branch. The error is caused by the following:

  1. HDDS-3597 eliminated the need for installing Protobuf, now we use protobuf-maven-plugin to compile .proto files.
  2. HDDS-3757 changed acceptance check to build directly on GitHub's runner. Previously it was built inside a docker container which comes with Protobuf pre-installed.
  3. HDDS-3621 moved proto files to separate subprojects.
  4. HDDS-3186 introduced SCMRatisProtocol.proto on the HDDS-2823 branch, in the old location and using hadoop-maven-plugins:protoc to compile it. (It was the correct location and plugin at that time.)
  5. 82c30a4 merged the above 1-3 changes from master to HDDS-2823, so now protoc is no longer available when building Ozone for acceptance. Other checks are still run in the docker container, so they are not affected.

The fix is to move new SCMRatisProtocol.proto to hadoop-hdds/interface-server project, and let it be compiled by protobuf-maven-plugin.

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

How was this patch tested?

Compiled locally after uninstalling Protobuf:

$ which protoc
protoc not found

$ mvn -DskipTests clean package
...
[INFO] BUILD SUCCESS

Acceptance test compiles fine:
https://github.com/adoroszlai/hadoop-ozone/runs/826834813#step:5:8603

@adoroszlai adoroszlai self-assigned this Jul 1, 2020
@adoroszlai adoroszlai requested a review from nandakumar131 July 1, 2020 15:24
@timmylicheng
Copy link
Contributor

Is this happening for master as well?

@adoroszlai
Copy link
Contributor Author

Is this happening for master as well?

No, it's specific to the HDDS-2823 branch. I've updated the PR description with an explanation.

@nandakumar131
Copy link
Contributor

The test failure doesn't seem to be related. Thanks @adoroszlai for the quick fix.

@nandakumar131
Copy link
Contributor

I have retriggered the prechecks.

Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

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

Change LGTM, +1 pending CI.

@timmylicheng
Copy link
Contributor

I will merge this. Thanks @adoroszlai for the contribution.

@timmylicheng timmylicheng merged commit 144f9a8 into apache:HDDS-2823 Jul 3, 2020
@adoroszlai adoroszlai deleted the HDDS-3911 branch July 3, 2020 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants