Fix dependencies to run TcpServerBM from command line #280
Conversation
| @@ -25,4 +25,7 @@ | |||
|
|
|||
| provides io.aklivity.zilla.runtime.engine.config.ConditionConfigAdapterSpi | |||
| with io.aklivity.zilla.runtime.binding.tcp.internal.config.TcpConditionConfigAdapter; | |||
|
|
|||
| provides jakarta.json.spi.JsonProvider | |||
| with org.leadpony.joy.yaml.YamlProvider; | |||
There was a problem hiding this comment.
This should not be necessary because we don't use modules for the shaded test jar.
| @@ -0,0 +1 @@ | |||
| org.leadpony.joy.yaml.YamlProvider | |||
There was a problem hiding this comment.
Replicating this here doesn't seem correct, it should be coming from the engine JAR when shading instead.
| <include>org.leadpony.joy:joy-yaml</include> | ||
| <include>org.leadpony.joy:joy-core</include> | ||
| <include>org.snakeyaml:snakeyaml-engine</include> | ||
| <include>com.ibm.icu:icu4j</include> |
There was a problem hiding this comment.
Is there any shade plugin configuration to pull transitive dependencies implicitly?
Otherwise this will be brittle as we change the dependencies in engine over time.
| { | ||
| System.out.println("warning: json syntax is deprecated, migrate to yaml"); | ||
| } | ||
| } |
There was a problem hiding this comment.
We don't need to replicate this backwards compatibility code from zilla start in the benchmark, please remove it.
The integration with OpenJMH will add support for picking up a configuration similar to ITs, so it won't be coming from command line or be file based.
| Properties properties = new Properties(); | ||
| properties.setProperty(ENGINE_DIRECTORY.name(), "target/zilla-benchmarks"); | ||
| properties.setProperty(ENGINE_STREAMS_BUFFER_CAPACITY.name(), Long.toString(1024L * 1024L * 16L)); | ||
| Properties props = new Properties(); |
There was a problem hiding this comment.
Please revert this rename back to properties.
runtime/binding-tcp/pom.xml
Outdated
| <include>org.openjdk.jmh:jmh-core</include> | ||
| <include>net.sf.jopt-simple:jopt-simple</include> | ||
| <include>org.apache.commons:commons-math3</include> | ||
| <include>commons-cli:commons-cli</include> | ||
| <include>com.github.biboudis:jmh-profilers</include> | ||
| </includes> | ||
| </artifactSet> | ||
| <includeDependencies>true</includeDependencies> |
There was a problem hiding this comment.
If this picks up dependencies transitively, can we trim the list above to no longer need to include any engine dependencies explicitly?
That would be better to implicitly keep the engine dependencies in sync.
There was a problem hiding this comment.
Also, suggest moving this configuration to top level pom.xml instead, with the rest of the default configuration for shade plugin as each project will need the same configuration.
| @@ -65,6 +65,9 @@ | |||
| properties.setProperty(ENGINE_DIRECTORY.name(), "target/zilla-benchmarks"); | |||
| properties.setProperty(ENGINE_STREAMS_BUFFER_CAPACITY.name(), Long.toString(1024L * 1024L * 16L)); | |||
|
|
|||
| properties.setProperty(ENGINE_DIRECTORY.name(), "target/zilla-benchmarks"); | |||
| properties.setProperty(ENGINE_STREAMS_BUFFER_CAPACITY.name(), Long.toString(1024L * 1024L * 16L)); | |||
|
|
|||
There was a problem hiding this comment.
These lines can now be removed as they duplicate the lines above, right?
| @@ -0,0 +1 @@ | |||
| org.leadpony.joy.yaml.YamlProvider | |||
There was a problem hiding this comment.
This file is already present in joy-yaml JAR, and not required by engine project otherwise.
If dependencies are being picked up correctly by shade plugin, shouldn't joy-yaml dependency cause this to be included in the shaded JAR without having to explicitly put it into our sources?
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)