Skip to content

feat: support subscribeMode field on pulsar #3831

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ericsyh
Copy link
Contributor

@ericsyh ericsyh commented May 22, 2025

Description

Please explain the changes you've made

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: #3832

Checklist

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

Signed-off-by: Eric Shen <ericshenyuhao@outlook.com>
@ericsyh ericsyh requested review from a team as code owners May 22, 2025 15:37
ericsyh added 2 commits May 23, 2025 10:00
Signed-off-by: Eric Shen <ericshenyuhao@outlook.com>
@msfussell
Copy link
Member

msfussell commented May 24, 2025

@ericsyh - Is there an issue number for this PR?

@ericsyh
Copy link
Contributor Author

ericsyh commented May 24, 2025

@ericsyh - Is there an issue number for this PR?

@msfussell sorry i missed the issue create before, just created a new issue of #3832 and linked on this PR.

nelson-parente
nelson-parente previously approved these changes May 26, 2025
Copy link
Contributor

@nelson-parente nelson-parente 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
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

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

Looking good to me, just a few nits

}
}

func getSubscriptionMode(subsModeStr string) pulsar.SubscriptionMode {
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see the addition of tests for this function to ensure future changes don't cause regressions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikeee Your comment makes sense, I added a new test func TestParsePulsarMetadataSubscriptionCombination which combines all existing subscription fields to validate the regressions in bf064af. PTAL

Co-authored-by: Mike Nguyen <hey@mike.ee>
Signed-off-by: Eric Shen <ericshenyuhao@outlook.com>
daixiang0 and others added 2 commits May 29, 2025 17:25
@ericsyh
Copy link
Contributor Author

ericsyh commented Jun 1, 2025

@nelson-parente @mikeee Could you help review again? cc @yaron2 @JoshVanL help review

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.

[feat](pulsar): support the subscription mode in PubSub Pulsar fields.
5 participants