search

apache / pekko-connectors

Apache Pekko Connectors is a Reactive Enterprise Integration library for Java and Scala, based on Reactive Streams and Apache Pekko.
https://pekko.apache.org/
Apache License 2.0
64 stars 32 forks source link

Enable inliner #395

Closed mdedetrich closed 9 months ago

mdedetrich commented 9 months ago

Enable the inliner since this is now 1.1.x. @pjfanning I know you were sensitive about explicitly setting this to true to make it more clear but doing so disables the knob/cli flag at https://github.com/pjfanning/sbt-pekko-build/blob/main/src/main/scala/com/github/pjfanning/pekkobuild/PekkoInlinePlugin.scala#L56 and I don't think explicitly setting pekkoInlineEnabled is having much of an effect either way on developers.

If you still want me to explicitly set it to true then let me know and ill update the PR.

mdedetrich commented 9 months ago

So although I have tested this in the past already so I am 99.99% sure there shouldn't be an issue, I will try and make every single CI for each connector pass before merging just to be ultra sure.

mdedetrich commented 9 months ago

I will wait until https://github.com/apache/incubator-pekko-connectors/pull/398 is resolved before merging this

He-Pin commented 9 months ago

I'd think explicit is better

mdedetrich commented 9 months ago

I'd think explicit is better

It removes the ability to disable the inliner locally which is useful if you want to debug issues

He-Pin commented 9 months ago

I'd think explicit is better

It removes the ability to disable the inliner locally which is useful if you want to debug issues

Maybe we can do this

pekkoInlineEnabled := !sys.props.contains("pekko.no.inline")
mdedetrich commented 9 months ago

Maybe we can do this

pekkoInlineEnabled := !sys.props.contains("pekko.no.inline")

Then we have to repeat this in every single Pekko project which may have inconsistency issues and also defeats the whole point of putting it in sbt-pekko-build, i.e. https://github.com/pjfanning/sbt-pekko-build/blob/main/src/main/scala/com/github/pjfanning/pekkobuild/PekkoInlinePlugin.scala#L55-L57 so it can be shared in all Pekko projects.

When we are able to release artifacts from CI then we can reverse the defaults so that inline isn't enable by default on local machines

He-Pin commented 9 months ago

Maybe we can do this

pekkoInlineEnabled := !sys.props.contains("pekko.no.inline")

Then we have to repeat this in every single Pekko project which may have inconsistency issues and also defeats the whole point of putting it in sbt-pekko-build, i.e. https://github.com/pjfanning/sbt-pekko-build/blob/main/src/main/scala/com/github/pjfanning/pekkobuild/PekkoInlinePlugin.scala#L55-L57 so it can be shared in all Pekko projects.

When we are able to release artifacts from CI then we can reverse the defaults so that inline isn't enable by default on local machines

Let's DRY

mdedetrich commented 9 months ago

@He-Pin There is also a related issue on this https://github.com/pjfanning/sbt-pekko-build/issues/9

mdedetrich commented 9 months ago

I am going to go ahead and merge this, I wanted to try and get a successful run of mqtt tests however after 90 runs I am sick of it and its well known that this test is incredibly flaky and hence its hit rate of success is really low. Even though the test is failing there is no observable in the way that this PR's mqtt tests fail and how in general the mqtt tests are failing.