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

Add Docs: smallrye-pulsar cannot publish message in sample Quarkus project #40628

Open
vszakd opened this issue May 14, 2024 · 13 comments
Open
Assignees
Labels
area/smallrye kind/enhancement New feature or request

Comments

@vszakd
Copy link

vszakd commented May 14, 2024

Description

Hello,
because of smallrye/smallrye-reactive-messaging#2567 (comment), the current Quarkus Documentation (https://quarkus.io/guides/pulsar-getting-started and https://quarkus.io/guides/pulsar) for smallrye-reactive-pulsar does not lead to a working project when Gradle is used.

The Documentation should mention that when Gradle is used, the following workaround has to be implemented smallrye/smallrye-reactive-messaging#2567 (comment).

Implementation ideas

No response

@vszakd vszakd added the kind/enhancement New feature or request label May 14, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented May 14, 2024

/cc @Ladicek (smallrye), @jmartisk (smallrye), @phillip-kruger (smallrye), @radcortez (smallrye)

@ozangunalp ozangunalp self-assigned this May 14, 2024
@gsmet
Copy link
Member

gsmet commented May 14, 2024

@ozangunalp is it a dependency that we declare? If so would it be better if we were explicit about wanting the jar in the pom file?

@ozangunalp
Copy link
Contributor

ozangunalp commented May 14, 2024

@gsmet It is a transitive dependency. Maven resolves it correctly to jar but not Gradle.
I am not sure if adding explicit dependencies to jar artifacts would resolve the issue.
And even though it resolves the issue, I am hesitant to add them because it adds yet another dependency version to synchronize.

@gsmet
Copy link
Member

gsmet commented May 14, 2024

Yeah if it comes as a transitive dependency, I think I would rather try to ping the upstream projects so that they explicitly require a jar in the pom (given this particular artifact is relatively specific, it makes sense to explicitly specify it). If it solves the problem, that would be better than documenting a workaround.

Of course, that will take time and in the meantime, we could document the workaround.

@humcqc
Copy link
Contributor

humcqc commented May 14, 2024

Hi @ozangunalp, @gsmet, I think this 2 org.apache.bookkeeper dependencies (circe-checksum and cpu-affinity) should be in sync with the pulsar-client.version define in quarkus.
So it will be better to keep the sync in quarkus, in the other way all quarkus users should sync it , and they will not know when to upgrade it, because they just use a quarkus version not a pulsar version.

@gsmet
Copy link
Member

gsmet commented May 14, 2024

But if they are transitive dependencies, they should come with versions supported by the components dragging them, right?

@humcqc
Copy link
Contributor

humcqc commented May 14, 2024

yes but more we delegate this to upper stream more work and errors we will have to set the correct version.

Example, the app is using quarkus 3.10, app doesn't know which version of pulsar-client is used and so they doesn't know the org.apache.bookkeeper dependencies.

If you document it , each time quarkus upgrade the pulsar client you will have to update the doc, migration doc ... And quarkus users will have to read the migration guide and apply the changes.
From my point of view it's the same as syncing the version in quarkus.

And syncing in quarkus will avoid a lot of late error in multiple apps that doesn't know the glue behind the pulsar extension.
WDYT ?

@ozangunalp
Copy link
Contributor

But if they are transitive dependencies, they should come with versions supported by the components dragging them, right?

I agree with this.

The upstream Apache Pulsar project manages that Bookkeeper dependency version. Unless there is a clear conflict between some core dependencies that will pull in Quarkus, I am against managing them in Quarkus.

This isn't because of anything we do in Quarkus, but how Gradle resolves the dependency.

I'll check the Pulsar project for a fix and update Quarkus docs with the workaround.

@gsmet
Copy link
Member

gsmet commented May 14, 2024

@humcqc yeah that's why I favor a fix upstream to use the proper dependency in Gradle too. Because you seem to suppose that it would be easy for us to keep track of things but we are handling a gazillion of dependencies. If we had to manage them all, that would just be a major maintenance issue for us.

So I agree that the situation is not ideal but if upstream can be fixed to actually handle Gradle properly, everything will be fine on your side without adding an additional maintenance burden on us.

@humcqc
Copy link
Contributor

humcqc commented May 14, 2024

yes ideally Apache Pulsar Client should handle it correctly.

But my remark is that the sync of the documentation of the workaround with the correct version is the same as the maintenance of the dependency itself.

But it's fine for me, I understand your point with all the dependencies you already have to handle. I hope you will not forgot to update the workaround documentation and the migration guide with the correct version.

Do you think you can add a gradle native test in quarkus, or if you already have one , add a pulsar test case.

And do you think quarkus should add the workaround when we create an app with the cli :
quarkus create app com.cqc:pulsar-native --extension='messaging-pulsar' --gradle

@gsmet
Copy link
Member

gsmet commented May 14, 2024

What I hope is that we will be able to drop the documentation soon enough (but I'm a dreamer :)).

@gsmet
Copy link
Member

gsmet commented May 14, 2024

Unfortunately, we cannot add Gradle tests for every Gradle corner case, especially if it only affects native as testing native is very slow.
I think we will have to live with it for now, even if not ideal.

At least, with proper doc and the discussion you started, people will find pointers to the solution relatively easily.

@humcqc
Copy link
Contributor

humcqc commented May 14, 2024

thanks @ozangunalp @gsmet for the quick replies, @ozangunalp could you link this issue and #40390 with the pulsar one.

And you can close #40390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/smallrye kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants