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

[MGDSTRM-9160] Process authentication and admin client construction as blocking #213

Merged
merged 2 commits into from
Aug 3, 2022

Conversation

MikeEdgar
Copy link
Contributor

Avoids complexity of running various subtasks of some operations on a worker thread.

@MikeEdgar MikeEdgar changed the title [MGDSTRM-9160] Process all endpoints as blocking operators [MGDSTRM-9160] Process all endpoints as blocking operations Jul 18, 2022
@MikeEdgar MikeEdgar force-pushed the MGDSTRM-9160 branch 7 times, most recently from f9ad0d8 to 40fd775 Compare July 29, 2022 13:08
@MikeEdgar MikeEdgar changed the title [MGDSTRM-9160] Process all endpoints as blocking operations [MGDSTRM-9160] Process authentication and end-points as blocking Jul 29, 2022
@MikeEdgar MikeEdgar marked this pull request as ready for review July 29, 2022 13:09
@MikeEdgar MikeEdgar added this to the 0.12.0 milestone Jul 29, 2022
@@ -37,6 +37,7 @@
import java.util.stream.Collectors;

@Path("/api/v1")
@Blocking
Copy link
Contributor

Choose a reason for hiding this comment

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

@MikeEdgar is there a performance impact here that we should test for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm looking into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up backing out this change and adjusting the createAdminClient method to return a CompletableFuture where the Kafka client creation occurs in on worker thread. Also the time allowed for processes running on an event loop thread was increased from the default of 2000ms to 4000ms. This seems reasonable for situations where the server is running with constrained CPU.

I was also successful in getting this to run with a native executable (a few other small changes) with very good performance.

@MikeEdgar MikeEdgar force-pushed the MGDSTRM-9160 branch 2 times, most recently from abfe654 to a8f5457 Compare July 29, 2022 20:37
Avoids running blocking operations (JWKS retrieval and Kafka Client
setup) on the Vert.x event loop/IO threads.
Minor adjustments to allow native executable:
- Add `@RegisterForReflection` to `OperationsHandler`
- Provide default to `kafka.admin.oauth.enabled`
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@MikeEdgar MikeEdgar changed the title [MGDSTRM-9160] Process authentication and end-points as blocking [MGDSTRM-9160] Process authentication and admin client construction as blocking Jul 29, 2022
@MikeEdgar MikeEdgar requested a review from vbusch August 1, 2022 13:32
Copy link
Contributor

@rareddy rareddy left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@vbusch vbusch left a comment

Choose a reason for hiding this comment

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

LGTM

@MikeEdgar MikeEdgar merged commit b45164c into bf2fc6cc711aee1a0c2a:main Aug 3, 2022
@MikeEdgar MikeEdgar deleted the MGDSTRM-9160 branch August 3, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants