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.

Sign up for GitHub

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

Jump to bottom

NOTIF-55 Migrate to resteasy-reactive and rest-client-reactive #202

Merged
merged 1 commit into from
May 18, 2021
Merged

NOTIF-55 Migrate to resteasy-reactive and rest-client-reactive #202

merged 1 commit into from
May 18, 2021

Conversation

gwenneg
Copy link
Member

@gwenneg gwenneg commented Apr 27, 2021 •
edited
Loading

Blocked by quarkusio/quarkus#16577: the x-rh-identity header is not sent because of a Quarkus bug, which leads to a failed RBAC call.

It should be fixed in Quarkus 1.13.3.Final.

@gwenneg
Copy link
Member Author

gwenneg commented Apr 29, 2021

Quarkus 1.13.3.Final was released yesterday.

quarkusio/quarkus#16577 was fixed as expected, but I found what seems to be a new bug: quarkusio/quarkus#16897

@gwenneg
Copy link
Member Author

gwenneg commented Apr 30, 2021

I worked around the bug and the build is now successful. Unfortunately, the response time is really bad (500 to 600ms now vs 10 to 100ms before) so I'm trying to figure out why the app is so slow.

@gwenneg gwenneg force-pushed the NOTIF-55 branch 7 times, most recently from 15eab90 to b51a6af Compare May 7, 2021 12:55
@gwenneg gwenneg mentioned this pull request May 10, 2021
Merged
@gwenneg gwenneg marked this pull request as ready for review May 10, 2021 08:21
@gwenneg
Copy link
Member Author

gwenneg commented May 10, 2021

⚠️ WARNING ⚠️
#235 must be merged before #202 to fix the response time issue.

All issues solved. This PR is ready for a review.

@gwenneg
Copy link
Member Author

gwenneg commented May 10, 2021

⚠️ Please contact me before merging this PR. I'll need to monitor prod after this is deployed.

@gwenneg
Copy link
Member Author

gwenneg commented May 17, 2021 •
edited
Loading

Rebased on master and tested with the merged preparation work (#235). It looks good.

This is ready for a review/merge.

josejulio
josejulio approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@josejulio josejulio 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!

josejulio
josejulio reviewed May 18, 2021
View reviewed changes
src/main/java/com/redhat/cloud/notifications/routers/InternalService.java
@Path("/internal")
@Consumes(APPLICATION_JSON)
@Produces(APPLICATION_JSON)
Copy link
Member

Choose a reason for hiding this comment

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

I thought this was the default and we didn't need to specify it?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true for quarkus-resteasy but apparently we have to add these annotations back with quarkus-reasteasy-reactive. I suspect the reactive extension is missing some bytecode enhancement. I'll investigate that later (or talk with the resteasy-reactive team).

@gwenneg gwenneg merged commit ac20874 into RedHatInsights:master May 18, 2021
@gwenneg gwenneg deleted the NOTIF-55 branch May 18, 2021 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josejulio josejulio josejulio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gwenneg @josejulio