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][fn] support reading config options from file in Function Python Runner #18951

Merged
merged 8 commits into from
Feb 8, 2023

Conversation

tpiperatgod
Copy link
Contributor

@tpiperatgod tpiperatgod commented Dec 16, 2022

Fixes #18533

Master Issue: #xyz

Motivation

please refer to this proposal: #18744

Modifications

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 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
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: tpiperatgod#11

@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2022

Codecov Report

Merging #18951 (5e4c381) into master (aa7af10) will decrease coverage by 1.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18951      +/-   ##
============================================
- Coverage     64.20%   63.17%   -1.03%     
+ Complexity    26265    25952     -313     
============================================
  Files          1832     1832              
  Lines        134067   134068       +1     
  Branches      14753    14753              
============================================
- Hits          86077    84704    -1373     
- Misses        40135    41661    +1526     
+ Partials       7855     7703     -152     
Flag Coverage Δ
inttests 24.79% <100.00%> (-0.13%) ⬇️
systests 25.56% <100.00%> (+0.09%) ⬆️
unittests 60.46% <100.00%> (-1.00%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 78.28% <100.00%> (+0.36%) ⬆️
...ain/java/org/apache/pulsar/broker/rest/Topics.java 0.00% <0.00%> (-100.00%) ⬇️
...ar/broker/stats/prometheus/ManagedLedgerStats.java 0.00% <0.00%> (-100.00%) ⬇️
...oker/stats/prometheus/PrometheusMetricStreams.java 0.00% <0.00%> (-100.00%) ⬇️
.../stats/prometheus/AggregatedSubscriptionStats.java 0.00% <0.00%> (-100.00%) ⬇️
...metheus/AggregatedTransactionCoordinatorStats.java 0.00% <0.00%> (-100.00%) ⬇️
...broker/stats/prometheus/TransactionAggregator.java 0.00% <0.00%> (-96.06%) ⬇️
.../pulsar/broker/rest/RestMessagePublishContext.java 0.00% <0.00%> (-92.60%) ⬇️
...a/org/apache/pulsar/client/util/TypeCheckUtil.java 0.00% <0.00%> (-83.34%) ⬇️
...ker/stats/prometheus/AggregatedNamespaceStats.java 0.00% <0.00%> (-81.25%) ⬇️
... and 202 more

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Jan 26, 2023
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.

could you please add some tests?

@tpiperatgod tpiperatgod force-pushed the issue-18533 branch 3 times, most recently from 93c64da to 660c540 Compare February 5, 2023 03:21
@github-actions github-actions bot removed the Stale label Feb 7, 2023
@nlu90 nlu90 merged commit 6506f9b into apache:master Feb 8, 2023
@momo-jun
Copy link
Contributor

Hi @tpiperatgod, did you have any plans to update the related docs?

@tpiperatgod
Copy link
Contributor Author

Hi @tpiperatgod, did you have any plans to update the related docs?

I will try to submit a PR for the document, please help me review it then, thanks.

@momo-jun
Copy link
Contributor

@tpiperatgod I'm happy to help. Feel free to ping me.

@tpiperatgod tpiperatgod deleted the issue-18533 branch February 13, 2023 10:12
@Anonymitaet
Copy link
Member

Hi @tpiperatgod 3.0 is planned to be released on May 2. We're aiming to align docs with code. Can you share some progress on the doc? TIA!

Hi @tpiperatgod, did you have any plans to update the related docs?

I will try to submit a PR for the document, please help me review it then, thanks.

@tpiperatgod
Copy link
Contributor Author

@Anonymitaet Here is the docs pr, please help review: apache/pulsar-site#541

@Anonymitaet
Copy link
Member

@tpiperatgod thanks for your technical input! I've restructured the docs based on user learning habits and raised a question in apache/pulsar-site#544 (comment), PTAL.

@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/function doc-complete Your PR changes impact docs and the related docs have been already added. ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Support reading config options from file in Function Python Runner
7 participants