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.

Sign up for GitHub

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

Jump to bottom

Allow the option to make producers thread local #7764

Merged
merged 2 commits into from
Aug 10, 2020
Merged

Allow the option to make producers thread local #7764

merged 2 commits into from
Aug 10, 2020

Conversation

srkukarni
Copy link
Contributor

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

If a function has many threads who are doing context.newOutputMessage().send(), there could be a lot of contention due to big synchronization blocks in the ProducerImpl. By allowing functions to use thread local producers, this synchronization can be avoided leading to increased performance.
This pr adds the configurability of using thread local producers in functions and sources

Modifications

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

(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:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@srkukarni srkukarni added this to the 2.7.0 milestone Aug 5, 2020
@srkukarni srkukarni self-assigned this Aug 5, 2020
sijie
sijie requested changes Aug 6, 2020
View reviewed changes
pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/ContextImpl.java
if (config.getFunctionDetails().getSink().getProducerSpec() != null) {
if (config.getFunctionDetails().getSink().getProducerSpec().getMaxPendingMessages() != 0) {
this.producerBuilder.maxPendingMessages(config.getFunctionDetails().getSink().getProducerSpec().getMaxPendingMessages());
}
if (config.getFunctionDetails().getSink().getProducerSpec().getMaxPendingMessagesAcrossPartitions() != 0) {
this.producerBuilder.maxPendingMessagesAcrossPartitions(config.getFunctionDetails().getSink().getProducerSpec().getMaxPendingMessagesAcrossPartitions());
}
useThreadLocalProducers = config.getFunctionDetails().getSink().getProducerSpec().getUseThreadLocalProducers();
Copy link
Member

Choose a reason for hiding this comment

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

This would cause a NullPointerException if useThreadLocalProducers is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useThreadLocalProducers is a bool in protobuf. Its a primitive value which is always going to be present with a default value

@srkukarni srkukarni requested a review from sijie August 7, 2020 16:22
@srkukarni
Copy link
Contributor Author

@sijie PTALA

@codelipenghui codelipenghui merged commit a3eb556 into apache:master Aug 10, 2020
@srkukarni srkukarni deleted the thread_local_producers branch August 10, 2020 15:45
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@srkukarni @huangdx0726
Allow the option to make producers thread local (apache#7764)
783800d 
If a function has many threads who are doing context.newOutputMessage().send(), there could be a lot of contention due to big synchronization blocks in the ProducerImpl. By allowing functions to use thread local producers, this synchronization can be avoided leading to increased performance.
This pr adds the configurability of using thread local producers in functions and sources

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
jerrypeng pushed a commit to jerrypeng/incubator-pulsar that referenced this pull request Aug 24, 2020
@srkukarni
Allow the option to make producers thread local (apache#7764)
b0c2056 
If a function has many threads who are doing context.newOutputMessage().send(), there could be a lot of contention due to big synchronization blocks in the ProducerImpl. By allowing functions to use thread local producers, this synchronization can be avoided leading to increased performance.
This pr adds the configurability of using thread local producers in functions and sources

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
@srkukarni
Allow the option to make producers thread local (apache#7764)
b6f2efa 
If a function has many threads who are doing context.newOutputMessage().send(), there could be a lot of contention due to big synchronization blocks in the ProducerImpl. By allowing functions to use thread local producers, this synchronization can be avoided leading to increased performance.
This pr adds the configurability of using thread local producers in functions and sources

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
@srkukarni
Allow the option to make producers thread local (apache#7764)
40b1aae 
If a function has many threads who are doing context.newOutputMessage().send(), there could be a lot of contention due to big synchronization blocks in the ProducerImpl. By allowing functions to use thread local producers, this synchronization can be avoided leading to increased performance.
This pr adds the configurability of using thread local producers in functions and sources

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
@srkukarni
Allow the option to make producers thread local (apache#7764)
6975616 
If a function has many threads who are doing context.newOutputMessage().send(), there could be a lot of contention due to big synchronization blocks in the ProducerImpl. By allowing functions to use thread local producers, this synchronization can be avoided leading to increased performance.
This pr adds the configurability of using thread local producers in functions and sources

Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sijie sijie sijie approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@jerrypeng jerrypeng Awaiting requested review from jerrypeng

Assignees

@srkukarni srkukarni

Labels
area/function
Projects
None yet
Milestone
2.7.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@srkukarni @sijie @codelipenghui