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

[fix][cli] Fix timeout behavior of reader perf #17882

Merged
merged 4 commits into from
Nov 2, 2022

Conversation

labuladong
Copy link
Contributor

@labuladong labuladong commented Sep 29, 2022

Fixes #17824

Modifications

Add a timer to handle timeout exit.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests.

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

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

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Matching PR in forked repository

PR in forked repository: labuladong#1

@github-actions github-actions bot added the doc-complete Your PR changes impact docs and the related docs have been already added. label Sep 29, 2022
@labuladong labuladong closed this Sep 29, 2022
@labuladong labuladong reopened this Sep 29, 2022
@labuladong labuladong marked this pull request as draft September 29, 2022 02:58
@labuladong labuladong marked this pull request as ready for review September 29, 2022 03:09
@labuladong
Copy link
Contributor Author

In the listener, all the actions are concurrent safe, so I just add a timer to handle timeout.

@mattisonchao @andFrasbeni PTAL

@labuladong
Copy link
Contributor Author

@andrasbeni @mattisonchao Could you please review this pr? Thanks

Copy link
Contributor

@andrasbeni andrasbeni left a comment

Choose a reason for hiding this comment

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

This fix looks good to me

@labuladong
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@tisonkun
Copy link
Member

You may change the PR title as [fix][test] ....

@labuladong
Copy link
Contributor Author

But it's about the perf tool, not a test.

@tisonkun
Copy link
Member

@labuladong I think we currently accept the following scope, you may choose one or propose changes:

# Scope abbreviation comments:
# cli -> command line interface
# fn -> Pulasr Functions
# io -> Pulsar Connectors
# offload -> tiered storage
# sec -> security
# sql -> Pulsar Trino Plugin
# txn -> transaction
# ws -> websocket
scopes: |
admin
broker
build
ci
cli
client
doc
fn
io
meta
misc
monitor
offload
proxy
schema
sec
site
sql
storage
test
txn
ws

@labuladong labuladong changed the title [fix][perf] Fix timeout behavior of reader perf [fix][cli] Fix timeout behavior of reader perf Oct 17, 2022
@labuladong
Copy link
Contributor Author

I think this pr belongs to cli scope, thanks. @tisonkun

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2022

Codecov Report

Merging #17882 (4754b3f) into master (0866c3a) will increase coverage by 10.75%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #17882       +/-   ##
=============================================
+ Coverage     38.97%   49.72%   +10.75%     
+ Complexity     8311     7021     -1290     
=============================================
  Files           683      400      -283     
  Lines         67325    43612    -23713     
  Branches       7217     4477     -2740     
=============================================
- Hits          26239    21686     -4553     
+ Misses        38079    19501    -18578     
+ Partials       3007     2425      -582     
Flag Coverage Δ
unittests 49.72% <ø> (+10.75%) ⬆️

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

Impacted Files Coverage Δ
...java/org/apache/pulsar/proxy/stats/TopicStats.java 58.82% <0.00%> (-41.18%) ⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java 3.26% <0.00%> (-39.87%) ⬇️
...ersistentStickyKeyDispatcherMultipleConsumers.java 44.71% <0.00%> (-12.99%) ⬇️
...e/pulsar/broker/service/EntryBatchIndexesAcks.java 82.14% <0.00%> (-10.72%) ⬇️
...ce/schema/validator/StructSchemaDataValidator.java 52.38% <0.00%> (-9.53%) ⬇️
...org/apache/pulsar/broker/loadbalance/LoadData.java 58.33% <0.00%> (-8.34%) ⬇️
...g/apache/pulsar/broker/service/StreamingStats.java 75.67% <0.00%> (-8.11%) ⬇️
.../pulsar/broker/service/PublishRateLimiterImpl.java 58.00% <0.00%> (-6.00%) ⬇️
...ava/org/apache/pulsar/utils/StatsOutputStream.java 65.45% <0.00%> (-5.46%) ⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java 59.15% <0.00%> (-5.11%) ⬇️
... and 318 more

@tisonkun
Copy link
Member

/pulsarbot rerun-failure-checks

1 similar comment
@labuladong
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@labuladong
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@codelipenghui codelipenghui added this to the 2.12.0 milestone Nov 1, 2022
@codelipenghui codelipenghui added the type/bug The PR fixed a bug or issue reported a bug label Nov 1, 2022
@labuladong
Copy link
Contributor Author

Could you please merge this pr? @codelipenghui

@codelipenghui codelipenghui merged commit 2e878e8 into apache:master Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli doc-complete Your PR changes impact docs and the related docs have been already added. type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] PerformanceConsumer and PerformanceReader run with -time quit only after another message
7 participants