-
Notifications
You must be signed in to change notification settings - Fork 336
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
[improve] PIP-368: Support lookup based on the lookup properties #1303
Conversation
Could you use a separated PR to fix conflicts after upgrading Pulsar to 4.0.0 like #1302? BTW, I found the lint task in the Makefile is repeated, we use |
You are right, |
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.
Could you use a separated PR to fix conflicts after upgrading Pulsar to 4.0.0?
In addition, we'd better use latest image by default.
Sorry, I didn't fully understand what you meant😂. This PR has upgraded the image to 4.0.0. do you mean to create a separate PR for this part? I think the contents of this PR are not complicated, and the code review should be completed quickly. |
Yes. Including the tool versions upgrade in a code PR is not a good practice. |
PTAL @BewareMyPower . #1304 |
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.
LGTM, could you add a test for verify the lookup properties in the pulsar/internal/lookup_service_test.go
?
Relevant code has been added https://github.com/apache/pulsar-client-go/pull/1303/files#diff-eaa8996b10b63f4ccd290c3af5dff5397e3911ab7cd228e20ecc99f0d69aeff4. |
Motivation
Support pip: apache/pulsar#23223
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation