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

Support configurable MaxOutstanding* and NumGoroutines settings for GCP PubSub component #3442

Merged

Conversation

nathandl86
Copy link
Contributor

@nathandl86 nathandl86 commented Jun 11, 2024

Description

Adds metadata settings for GCP PubSub component to configure the MaxOutstandingMessages, MaxOutstandingBytes, and NumGoroutines settings in the underlying Google Cloud pubsub package.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #3441

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@nathandl86 nathandl86 requested review from a team as code owners June 11, 2024 21:16
@berndverst
Copy link
Member

If a work in progress, please open as a draft PR. I'll convert to draft for now. (Most maintainers do not review draft PRs by the way)

@berndverst berndverst marked this pull request as draft June 11, 2024 23:55
@berndverst
Copy link
Member

Can you explore adding some sort of test that shows these settings are used correctly? Our "certification test" (which is a kind of end to end integration test) lives here: https://github.com/dapr/components-contrib/tree/main/tests/certification/pubsub/gcp/pubsub

@nathandl86
Copy link
Contributor Author

If a work in progress, please open as a draft PR. I'll convert to draft for now. (Most maintainers do not review draft PRs by the way)

Ah, sorry about that 🙇🏻 I'll add a commit updating the CONTRIBUTING.md to call out that the PR should be draft where it mentions the [WIP] prefix. I wasn't able to add the do not merge label. Do you want that removed from the readme?

@nathandl86
Copy link
Contributor Author

Can you explore adding some sort of test that shows these settings are used correctly? Our "certification test" (which is a kind of end to end integration test) lives here: https://github.com/dapr/components-contrib/tree/main/tests/certification/pubsub/gcp/pubsub

Yep! I'm going to look into adding the tests tonight. Part of the reason I opened it as [WIP] was I knew that wasn't there. The other reason it's currently a [WIP] is I haven't been able to manually test yet either.

@nathandl86
Copy link
Contributor Author

Can you explore adding some sort of test that shows these settings are used correctly? Our "certification test" (which is a kind of end to end integration test) lives here: https://github.com/dapr/components-contrib/tree/main/tests/certification/pubsub/gcp/pubsub

Yep! I'm going to look into adding the tests tonight. Part of the reason I opened it as [WIP] was I knew that wasn't there. The other reason it's currently a [WIP] is I haven't been able to manually test yet either.

@berndverst One hang-up I'm running into is how to iterate on the tests to prove this out. The certification tests look tied to GitHub Actions. Is there a way y'all typically run/iterate on these locally? It's entirely possible this is just me not being as comfortable in golang 😅

@yaron2
Copy link
Member

yaron2 commented Jun 17, 2024

@nathandl86 we don't have certification test infrastructure for GCP pub/sub today, so you're not required to add a test for this that interacts with GCP infra. You can update the existing Init certification test to check for the new metadata field, and this test you can run locally.

@nathandl86 nathandl86 force-pushed the configurable-gcp-pubsub-max-outstanding branch from 82f7bbe to b639daf Compare June 21, 2024 14:34
@nathandl86
Copy link
Contributor Author

@nathandl86 we don't have certification test infrastructure for GCP pub/sub today, so you're not required to add a test for this that interacts with GCP infra. You can update the existing Init certification test to check for the new metadata field, and this test you can run locally.

@yaron2 I updated the unit tests living next to the GCP pubsub module. I'm not sure if those are the Init tests you intended though since they did not live in the certification directory?

@nathandl86 nathandl86 changed the title [WIP] Support configurable MaxOutstanding* settings for GCP PubSub component [WIP] Support configurable MaxOutstanding* and NumGoroutine settings for GCP PubSub component Jun 21, 2024
@nathandl86 nathandl86 changed the title [WIP] Support configurable MaxOutstanding* and NumGoroutine settings for GCP PubSub component [WIP] Support configurable MaxOutstanding* and NumGoroutines settings for GCP PubSub component Jun 21, 2024
@nathandl86
Copy link
Contributor Author

I'd mentioned in the Discord that we had some challenges in manual testing. Closing that loop, I worked with my DevOps team and they setup an isolated cluster for us to test. Testing was successful 🎉 Where before we were seeing scale up of pods that were not processing messages,
image

We now see full participation of our pods because there are available messages for their sidecar to be served 🙌🏻
image

We also saw a massive improvement in our unacked age metric. Before we would see it climb above 20 minutes during a load test.
image

Across 5 tests with higher volume yesterday and today it stayed just below 4:
image

pubsub/gcp/pubsub/pubsub.go Outdated Show resolved Hide resolved
Signed-off-by: Nathan Lowry <nathandl@gmail.com>
Signed-off-by: Nathan Lowry <nathandl@gmail.com>
Signed-off-by: Nathan Lowry <nathandl@gmail.com>
Signed-off-by: Nathan Lowry <nathandl@gmail.com>
Signed-off-by: Nathan Lowry <nathandl@gmail.com>
Signed-off-by: Nathan Lowry <nathandl@gmail.com>
Signed-off-by: Nathan Lowry <nathandl@gmail.com>
Signed-off-by: Nathan Lowry <nathandl@gmail.com>
@nathandl86 nathandl86 force-pushed the configurable-gcp-pubsub-max-outstanding branch from 4ffd9c7 to c8e03f2 Compare June 21, 2024 18:45
@nathandl86 nathandl86 marked this pull request as ready for review June 21, 2024 18:45
@nathandl86 nathandl86 changed the title [WIP] Support configurable MaxOutstanding* and NumGoroutines settings for GCP PubSub component Support configurable MaxOutstanding* and NumGoroutines settings for GCP PubSub component Jun 21, 2024
@ItalyPaleAle ItalyPaleAle added this to the v1.14 milestone Jun 21, 2024
@ItalyPaleAle ItalyPaleAle merged commit 864baaa into dapr:main Jun 21, 2024
88 of 90 checks passed
@ItalyPaleAle
Copy link
Contributor

ItalyPaleAle commented Jun 21, 2024

Thank you!

Can you please update the docs in https://github.com/dapr/docs too?

@ItalyPaleAle ItalyPaleAle added the documentation required This issue needs documentation label Jun 21, 2024
@nathandl86
Copy link
Contributor Author

Thank you!

Can you please update the docs in https://github.com/dapr/docs too?
Thank you for the feedback and merging it 🙇🏻

Yep! I'll work on that now. A couple things I want to confirm:

@ItalyPaleAle
Copy link
Contributor

Yes to both, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation required This issue needs documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal for GCP PubSub Component: Support Configurable MaxOutstanding* Settings
4 participants