Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build from submodules (broker, transaction coordinator) #11795

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

nicoloboschi
Copy link
Contributor

Motivation

Currently running mvn install -DskipTests from pulsar/pulsar-broker and pulsar/pulsar-transaction/coordinator is failing due to (see also #11030)


[ERROR] Failed to generate lightproto code for [/home/hangc/Downloads/tmp/pulsar/pulsar-broker/src/main/proto/TransactionPendingAck.proto, /home/hangc/Downloads/tmp/pulsar/pulsar-broker/src/main/proto/ResourceUsage.proto]: java.lang.IllegalStateException: Imported proto pulsar-common/src/main/proto/PulsarApi.proto not found. (null)
java.lang.RuntimeException: java.lang.IllegalStateException: Imported proto pulsar-common/src/main/proto/PulsarApi.proto not found. (null)
    at io.protostuff.parser.Proto.importProto (Proto.java:316)
    at io.protostuff.parser.ProtoParser.header_import (ProtoParser.java:1431)
    at io.protostuff.parser.ProtoParser.statement (ProtoParser.java:336)
    at io.protostuff.parser.ProtoParser.parse (ProtoParser.java:165)
    at io.protostuff.parser.ProtoUtil.loadFrom (ProtoUtil.java:52)
    at io.protostuff.parser.ProtoUtil.loadFrom (ProtoUtil.java:60)
    at io.protostuff.parser.ProtoUtil.loadFrom (ProtoUtil.java:90)
    at com.github.splunk.lightproto.generator.LightProtoGenerator.generate (LightProtoGenerator.java:42)
    at com.github.splunk.lightproto.maven.plugin.LightProtoMojo.generate (LightProtoMojo.java:62)
    at com.github.splunk.lightproto.maven.plugin.LightProtoMojo.execute (LightProtoMojo.java:94)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:137)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:210)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:156)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:148)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:117)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:81)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:56)
    at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:128)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:305)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:192)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:105)

Modifications

Added maven-properties-plugin on initalize Maven phase, which sets two system properties used by protostuff (used by protolight plugin) 

  • proto_path: added the root directory in order to resolve imports like pulsar-common/src/main/proto/PulsarApi.proto
  • proto_search_strategy: value 2 (default value is not working, I feel it's buggy)

see https://github.com/protostuff/protostuff/blob/master/protostuff-parser/src/main/java/io/protostuff/parser/DefaultProtoLoader.java for more context

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): yes, but a compile time (added maven-properties-plugin)
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

For this PR, do we need to update docs?

Not needed

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Please revert formatting

pulsar-transaction/coordinator/pom.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@hangc0276 @lhotari PTAL

Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

Good job!

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli eolivelli merged commit ea1e0f4 into apache:master Aug 26, 2021
@eolivelli
Copy link
Contributor

merged to master branch, nice work @nicoloboschi

codelipenghui pushed a commit that referenced this pull request Sep 9, 2021
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Sep 9, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
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.

5 participants