-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Upgrade Smallrye Reactive Messaging to 3.8.0 #18972
Upgrade Smallrye Reactive Messaging to 3.8.0 #18972
Conversation
|
||
| *group.id* | A unique string that identifies the consumer group the application belongs to. If not set, a unique, generated id is used | ||
|
||
Type: _string_ | false | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to figure out this sync issue, because everytime we sync from SmallRye, we lose this important piece of content:
| *group.id* | A unique string that identifies the consumer group the application belongs to.
If not set, defaults to the application name as set by the `quarkus.application.name` configuration property.
If that is not set either, a unique, generated id is used.
It is recommended to always define a `group.id`, the automatic generation is only a convenient feature for development.
Type: _string_ | false |
(We should actually update this to mention ${quarkus.uuid}
, but first, we need to make sure we don't lose the text all the time.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested a "not-so-ugly" way of doing this. But it involves generating optional includes in the original SmallRye RM documentation. @Ladicek what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it needs to be includes, maybe just AsciiDoc conditionals (ifdef
, ifndef
) would be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that neither include
nor ifdef
solves this issue. I don't think that we should add quarkus related content to the SmallRye RM repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included the ${quarkus.uuid}
to group.id
property documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we already have Quarkus-specific stuff in RM documentation, so I would be fine with including more, but your call :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know I can hardly find any reference to Quarkus in RM repo. Maybe we can decide on this later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing!
This looks OK to me. I didn't know we don't have the necessary infrastructure in place for |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building f26cfb7
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/hibernate-reactive-panache✖
📦 integration-tests/kubernetes/quarkus-standard-way✖
✖
✖
✖
✖
✖
|
f26cfb7
to
f840d17
Compare
@xstefank can you take a look at this for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM. It seems that I missed the stuff that Quarkus does on top of the spec integration when I was doing the update so thanks for this.
|
||
It is recommended to always define a `group.id`, the automatic generation is only a convenient feature for development. | ||
|
||
Type: _string_ | false | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So originally, we had
| *group.id* | A unique string that identifies the consumer group the application belongs to.
If not set, defaults to the application name as set by the `quarkus.application.name` configuration property.
If that is not set either, a unique, generated id is used.
It is recommended to always define a `group.id`, the automatic generation is only a convenient feature for development.
Type: _string_ | false |
What we have now is a little hard to understand (the mention of automatic generation without previous explanation is strange, and the sentence about quarkus.uuid
interrupts the reading flow IMHO).
I'd add the quarkus.uuid
stuff at the end, resulting in something like:
| *group.id* | A unique string that identifies the consumer group the application belongs to.
If not set, defaults to the application name as set by the `quarkus.application.name` configuration property.
If that is not set either, a unique, generated id is used.
It is recommended to always define a `group.id`, the automatic generation is only a convenient feature for development.
You can explicitly ask for automatically generated unique id by setting this property to `${quarkus.uuid}`.
Type: _string_ | false |
I wouldn't be nitpicking now, except I see this needs to be rebased anyway, so I thought it won't hurt :-) |
I know, I was waiting for your final word :) |
Wire startup probe
…cumentation Update health check doc for readiness and startup Add record key propagation.
f840d17
to
bbe8e4e
Compare
OK :-) Sorry it took me so long! |
Wire startup probe
Update documentation for health checks and record key propagation
Copy up-to-date configuration documentation from reactive messaging.