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

[improve][admin] Put validateTopicOwnershipAsync before validateTopicOperationAsync #15265

Merged
merged 1 commit into from
Apr 22, 2022

Conversation

Technoboy-
Copy link
Contributor

@Technoboy- Technoboy- commented Apr 22, 2022

Motivation

Maybe for mistake or other reason, some patches have put validateTopicOperationAsync first then validateTopicOwnershipAsync. Related patches : #13880, #13878, #14045, #15120, #13845, #13805
If the topic ownership does not belong to this broker, it will redirect to another broker, and validateTopicOperationAsync is useless in this case. so we should put validateTopicOwnershipAsync before validateTopicOperationAsync.

Documentation

  • no-need-doc

@Technoboy- Technoboy- changed the title [improve][broker] Put validateTopicOwnershipAsync before validateTopicOperationAsync [improve][admin] Put validateTopicOwnershipAsync before validateTopicOperationAsync Apr 22, 2022
@Technoboy- Technoboy- added the doc-not-needed Your PR changes do not impact docs label Apr 22, 2022
@lhotari
Copy link
Member

lhotari commented Apr 22, 2022

Motivation

Put validateTopicOwnershipAsync before validateTopicOperationAsync to decrease validateTopicOperationAsync one time.

@Technoboy- please improve the description. What does this mean?

It looks like this is an improvement for a recent change #15120. It would be worth cross-referencing that in the description to provide more context.

@Technoboy-
Copy link
Contributor Author

Motivation

Put validateTopicOwnershipAsync before validateTopicOperationAsync to decrease validateTopicOperationAsync one time.

@Technoboy- please improve the description. What does this mean?

It looks like this is an improvement for a recent change #15120. It would be worth cross-referencing that in the description to provide more context.

Yes, it's one of the patches.

@lhotari
Copy link
Member

lhotari commented Apr 22, 2022

Yes, it's one of the patches.

Please be more specific and reference the other patches if you have that information. One reason why this is important is when someone is cherry-picking one of those patches, they know that the patch isn't complete and should include this change.

It's better to provide more context in pull request descriptions since reviewers need the context information for reviewing the PR. Every reviewer has to lookup the context on their own if you don't provide it. That's a major waste of time which could easily be eliminated by providing better descriptions and more context.

@Technoboy-
Copy link
Contributor Author

Yes, it's one of the patches.

Please be more specific and reference the other patches if you have that information. One reason why this is important is when someone is cherry-picking one of those patches, they know that the patch isn't complete and should include this change.

It's better to provide more context in pull request descriptions since reviewers need the context information for reviewing the PR. Every reviewer has to lookup the context on their own if you don't provide it. That's a major waste of time which could easily be eliminated by providing better descriptions and more context.

Yes, agree. I will update the description later.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, good work @Technoboy-

@lhotari lhotari merged commit 41f40f0 into apache:master Apr 22, 2022
codelipenghui pushed a commit that referenced this pull request Apr 28, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Apr 28, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 9, 2022
@Technoboy- Technoboy- deleted the fix-rest-validate-order branch August 10, 2022 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants