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][functions]Adding boolean in contextImpl.java to check partitioned topic present or not. #23407

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nikam14
Copy link
Contributor

@nikam14 nikam14 commented Oct 7, 2024

Motivation

In contextImpl.java the getConsumer() - When trying to get partitioned topic consumer it throws error consumer not found.
Instead show error message that partitioned topic not present.
Also reduces computation to search for consumer.

Modifications

Added a boolean var to check partitioned topic present or not.
when try to get consumer check whether partitioned topic present then proceed.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is already covered by existing tests, such as ContextImplTest.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 7, 2024
.forEach(consumer -> topicConsumers.putIfAbsent(TopicName.get(consumer.getTopic()), consumer));
.forEach(consumer -> {
topicConsumers.putIfAbsent(TopicName.get(consumer.getTopic()), consumer);
if (consumer.getTopic().contains("-partition-")) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use the isPartitioned() in TopicName class to determine whether the topic is partitioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review

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.

Thanks for the contribution @nikam14. Please check the review comments. In addition, there would have to be a test case to cover this. One possible location for the test case could be one of the classes in https://github.com/apache/pulsar/tree/master/pulsar-broker/src/test/java/org/apache/pulsar/io directory so that there would be a end-to-end test to cover the use case. In addition, a unit test could be added to https://github.com/apache/pulsar/blob/master/pulsar-functions/instance/src/test/java/org/apache/pulsar/functions/instance/ContextImplTest.java if it is feasible.

@lhotari
Copy link
Member

lhotari commented Oct 9, 2024

In contextImpl.java the getConsumer() - When trying to get partitioned topic consumer it throws error consumer not found.
Instead show error message that partitioned topic not present.
Also reduces computation to search for consumer.

@nikam14 Could you please provide more context about the specific use case where you encountered this issue? It would be helpful to understand:

  1. What were you trying to accomplish?
  2. What did you expect to happen?
  3. What actually happened instead?

This additional information will help us better understand the impact on end-users and ensure our solution addresses the root cause effectively.

@nikam14
Copy link
Contributor Author

nikam14 commented Oct 9, 2024

1 - With these changes when try to get partitonedTopicConsumer it will first check whether partitionedTopicConsumer exist or
not. If exist then return else throw error 'No Partitioned topic present' .
with these changes error message will be more specific.
2 - A more specific error message .
3 - If Consumer is not found in the Map then it returns consumer not found.

@lhotari
Copy link
Member

lhotari commented Oct 9, 2024

1 - With these changes when try to get partitonedTopicConsumer it will first check whether partitionedTopicConsumer exist or
not. If exist then return else throw error 'No Partitioned topic present' .
with these changes error message will be more specific.
2 - A more specific error message .
3 - If Consumer is not found in the Map then it returns consumer not found.

Please explain the context of the end user use case. As a pulsar end user, what is the use case? Please provide an example function that shows the problem.

@nikam14
Copy link
Contributor Author

nikam14 commented Oct 14, 2024

changes are covered in testGetConsumer()-ContextImplTest.
In testGetConsumer() test function we can see if user is trying to get a consumer of partitioned topic it will get error message that " no partitioned topic present ", which means there is no consumer with partitioned topic.

@lhotari
Copy link
Member

lhotari commented Oct 14, 2024

changes are covered in testGetConsumer()-ContextImplTest. In testGetConsumer() test function we can see if user is trying to get a consumer of partitioned topic it will get error message that " no partitioned topic present ", which means there is no consumer with partitioned topic.

Ok. What is the current error message without this change?

@lhotari
Copy link
Member

lhotari commented Oct 14, 2024

changes are covered in testGetConsumer()-ContextImplTest. In testGetConsumer() test function we can see if user is trying to get a consumer of partitioned topic it will get error message that " no partitioned topic present ", which means there is no consumer with partitioned topic.

this is not a end user test case. It's possible that you are making incorrect assumptions about the behavior as well as the reviewers. In Pulsar, the partitioned topics concepts can easily mislead anyone. Please see #20622 for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants