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

issue#9345 -- bug fix for RuntimeUtils.getCmd not setting pending-async-requests #9349

Merged

Conversation

csthomas1
Copy link
Contributor

Fixes #9345

Motivation

When I enable either the Process or Kubernetes function Runtimes and set the parameter "maxPendingAsyncRequests" to a value other than 1000 in the function worker configuration (e.g. functions_worker.yml), any function instances launched by the worker continue to have maxPendingAsyncRequests = 1000. I'd like to be able to adjust this parameter to guard against my function running out of memory -- without increasing the function heap size -- due to too many publish operations in flight, as can occur when the output topic's backlog quota has been reached.

Modifications

Modified RuntimeUtils.getCmd to add --pending-sync-requests to the list of arguments passed to JavaInstanceStarter

Verifying this change

This change added tests and can be verified as follows:

  • Updated pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntimeTest.java to test for presence of --pending-async-requests argument in generated commandline, and increased expected arg counts by 2 to account for expected presence of --pending-async-requests
  • Updated pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/runtime/process/ProcessRuntimeTest.java to test for presence of --pending-async-requests argument in generated commandline, and increased expected arg counts by 2 to account for expected presence of --pending-async-requests
  • Updated pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/worker/WorkerApiV2ResourceConfigTest.java to verify that the configured value of "maxPendingAsyncRequests" was being properly loaded into the WorkerConfig

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): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable
  • If a feature is not applicable for documentation, explain why?
    • bug fix to make an existing configuration parameter operational
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@codelipenghui
Copy link
Contributor

@nlu90 Please also help review this PR

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@csthomas1
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Renkai
Copy link
Contributor

Renkai commented Jan 30, 2021

Please merge the newest master and rerun tests

@csthomas1 csthomas1 force-pushed the bugfix_issue_9345_max_pending_async_requests branch from 5b2b4cf to e9d5078 Compare February 1, 2021 19:44
Copy link
Contributor

@freeznet freeznet left a comment

Choose a reason for hiding this comment

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

LGTM

@jerrypeng
Copy link
Contributor

@csthomas1 thanks for fix!

@csthomas1 csthomas1 force-pushed the bugfix_issue_9345_max_pending_async_requests branch from 9f32c2c to bbcc5af Compare February 2, 2021 22:21
Copy link
Member

@nlu90 nlu90 left a comment

Choose a reason for hiding this comment

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

LGTM

sorry for the late reply

…pending-async-requests argument supported by JavaInstanceStarter
@csthomas1 csthomas1 force-pushed the bugfix_issue_9345_max_pending_async_requests branch from bbcc5af to 52badee Compare February 4, 2021 18:52
@sijie
Copy link
Member

sijie commented Feb 11, 2021

/pulsarbot run-failure-checks

@sijie
Copy link
Member

sijie commented Feb 22, 2021

/pulsarbot run-failure-checks

1 similar comment
@zymap
Copy link
Member

zymap commented Feb 23, 2021

/pulsarbot run-failure-checks

@zymap
Copy link
Member

zymap commented Feb 24, 2021

@csthomas1 Looks like the test failed by this change. Could you please take a look?

move this to the next release.

@csthomas1
Copy link
Contributor Author

/pulsarbot run-failure-checks

@csthomas1
Copy link
Contributor Author

@zymap @sijie The tests are finally green for this PR...

@sijie sijie merged commit d2b45b6 into apache:master Mar 13, 2021
@sijie
Copy link
Member

sijie commented Mar 13, 2021

@csthomas1 Awesome! Thank you for your contribution!

fmiguelez pushed a commit to fmiguelez/pulsar that referenced this pull request Mar 16, 2021
…nc-requests (apache#9349)

Fixes apache#9345 

### Motivation

When I enable either the Process or Kubernetes function Runtimes and set the parameter "maxPendingAsyncRequests" to a value other than 1000 in the function worker configuration (e.g. functions_worker.yml), any function instances launched by the worker continue to have maxPendingAsyncRequests = 1000. I'd like to be able to adjust this parameter to guard against my function running out of memory -- without increasing the function heap size -- due to too many publish operations in flight, as can occur when the output topic's backlog quota has been reached.

### Modifications

Modified RuntimeUtils.getCmd to add --pending-sync-requests to the list of arguments passed to JavaInstanceStarter

### Verifying this change

This change added tests and can be verified as follows:


  - Updated pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntimeTest.java to test for presence of --pending-async-requests argument in generated commandline, and increased expected arg counts by 2 to account for expected presence of --pending-async-requests <value>
  - Updated pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/runtime/process/ProcessRuntimeTest.java to test for presence of --pending-async-requests argument in generated commandline, and increased expected arg counts by 2 to account for expected presence of --pending-async-requests <value>
  - Updated pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/worker/WorkerApiV2ResourceConfigTest.java to verify that the configured value of "maxPendingAsyncRequests" was being properly loaded into the WorkerConfig
@csthomas1 csthomas1 deleted the bugfix_issue_9345_max_pending_async_requests branch March 19, 2021 14:32
@csthomas1 csthomas1 restored the bugfix_issue_9345_max_pending_async_requests branch March 19, 2021 14:32
@eolivelli
Copy link
Contributor

this patch does not apply to branch-7.2

@csthomas1 csthomas1 deleted the bugfix_issue_9345_max_pending_async_requests branch June 15, 2021 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants