You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "jbarotin (via GitHub)" <gi...@apache.org> on 2024/03/01 11:35:23 UTC
[PR] [improve][client] add externalListenerThreadName parameter [pulsar]
jbarotin opened a new pull request, #22174:
URL: https://github.com/apache/pulsar/pull/22174
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://pulsar.apache.org/contribute/develop-semantic-title/)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
<!-- Either this PR fixes an issue, -->
Fixes #22170
### Motivation
Add a configuration option in the client to set the external-listener-thread name.
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
### Modifications
Modified client configuration
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- added a integration test to check the parameter is working
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*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
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "jbarotin (via GitHub)" <gi...@apache.org>.
jbarotin closed pull request #22174: [improve][client] add externalListenerThreadName parameter
URL: https://github.com/apache/pulsar/pull/22174
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1973029053
@jbarotin Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1997214271
> I think you can add an external executor provider config to `ClientBuilder.java`.
>
> It is more flexible.
@nodece There's a draft PIP, "PIP-234: Support using shared thread pool across multiple Pulsar client instance", for this purpose https://lists.apache.org/thread/5jw06hqlmwnrgvbn9lfom1vkwhwqwwd4 / https://github.com/apache/pulsar/issues/19074 . I don't think that the design is finished since there are some shaded class issues with the current design. We don't want to expose Netty in the Client API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1983927088
> But, even, it's a nice workaround, the app will lose coherence. I mean, if I execute a jstack, then the thread name printed will not be the same as the thread name written in logs. All my team members have to be aware of this workaround.
Well I'd consider this to not be a major issue.
There's a chance that the PIP doesn't get accepted as-is since we might not want to litter the.main interface with configuration options that are used by 1 user out of 10000.
One solution could be to have a way to hook into lower level configuration interfaces instead of adding all choices on the main interface. It would be great to see how such details are accommodated in other libraries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1997218699
> While developing this PR, I saw a misconception we had: we currently have one Pulsar Client by topic. My original idea was to use the executor thread name prefix to identify the topic name. I read around pulsar discussion that it's better to have one client instance by JVM instance.
Yes, it's better to share the instance. The only drawback currently is that rate limiting has a gap where all producers and consumers on the same connection get backpressured when any limit exceeds (mentioned in ["out of scope" for PIP-322](https://github.com/apache/pulsar/blob/master/pip/pip-322.md#out-of-scope)).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1982554307
Since the problem for you was logging configuration, isn't there a way to limit the length of the thread name in the logging configuration?
In Log4J, this is possible with something like `%.10t` to truncate the thread name to 10 characters. Please check [the docs for `%` in Log4J pattern layouts](https://logging.apache.org/log4j/2.x/manual/layouts.html#PatternPercentLiteral).
With Log4J, you could also replace a string to shorten it. Check ["replace" in the patterns part of the doc](https://logging.apache.org/log4j/2.x/manual/layouts.html#patterns).
For example, `%replace{%t}{pulsar-external-listener-}{pel-}` would replace `pulsar-external-listener-` with `pel-`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "jbarotin (via GitHub)" <gi...@apache.org>.
jbarotin commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1992096131
To be franc, my real use case is to display the "topic name" on every log line executed in the Pulsar external listener.
While developing this PR, I saw a misconception we had: we currently have one Pulsar Client by topic. My idea was to use the executor thread name prefix to identify the topic name. I read the we should better have one client instance by JVM instance.
This misconception comes from our historical message broker Apache ActiveMQ and its pool, because amq pool is working like this : one pool by "queue" (it's how topic are named in amq), and then pool thread name prefix is obtained from the Listener Interface we give to the pool, implementation of this interface has the same name the queue is consuming.
Anyway, have you got any idea how to display the topic name automatically in logs written by our code executed by the external-pulsar-listener ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1982550657
> The only thought is that I have is that if I could customize the external listener thread name prefix, why couldn't I customize the names of the other thread pools? I'd expect to have consistency across the API.
+1 to me. If this new method is merged, should we add some other methods like `setXxxThreadName` as well? Would that be "over-configured"? Note: Pulsar already has too many configurations that might never be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1979083454
I'd suggest renaming `externalListenerThreadName` to `externalListenerThreadNamePrefix`.
The value is currently passed to io.netty.util.concurrent.DefaultThreadFactory under the covers where the parameter is called `poolName`.
The actual thread name is in format `poolName-[poolIdCounter]-[threadIdCounter]`. This can be seen from the source code of DefaultThreadFactory: https://github.com/netty/netty/blob/85cee9d1a8881a93e7c43a02baefea75b6ade2b5/common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java#L93-L105
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "jbarotin (via GitHub)" <gi...@apache.org>.
jbarotin commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1982960096
> With Log4J, you could also replace a string to shorten it. Check ["replace" in the patterns part of the doc](https://logging.apache.org/log4j/2.x/manual/layouts.html#patterns).
> For example by using %replace{%t}{pulsar-external-listener-}{pel-} instead of %t in the logging pattern would result in pulsar-external-listener- getting replaced with pel- in the thread name.
For logging purpose, it should work, good idea, I'll try that.
But, even, it's a nice workaround, the app will not lose coherence. I mean, if I execute a `jstack`, then the thread name printed will not be the same as the thread name written in logs. All my team members have to be aware of this workaround.
I can try (next week) writing à PIP, in this way, and then, if it's accepted, update my PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][client] add externalListenerThreadName parameter [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22174:
URL: https://github.com/apache/pulsar/pull/22174#issuecomment-1984984379
I think you can add an external executor provider config to `ClientBuilder.java`.
It is more flexible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org