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

[fix][test] final shutdown executor when the class(PerformanceProducer) is destroyed #19926

Merged
merged 1 commit into from
Mar 29, 2023
Merged

[fix][test] final shutdown executor when the class(PerformanceProducer) is destroyed #19926

merged 1 commit into from
Mar 29, 2023

Conversation

StevenLuMT
Copy link
Member

@StevenLuMT StevenLuMT commented Mar 25, 2023 •
edited
Loading

Motivation

When the class(PerformanceProducer) destroy, shutdown the executor

Modifications

final shutdown executor when the class(PerformanceProducer) is destroyed

Verifying this change

  • Make sure that the change passes the CI checks.
    This change is a trivial rework / code cleanup without any test coverage.

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

Documentation

Check the box below or label this PR directly.

Need to update docs?

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

Matching PR in forked repository

PR in forked repository: StevenLuMT#7

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 25, 2023
tisonkun
tisonkun reviewed Mar 25, 2023
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Can you elaborate a bit why we need to explicitly shutdown the executor? If the process exits, all tasks just halted.

pulsar-testclient/src/main/java/org/apache/pulsar/testclient/PerformanceProducer.java Outdated
executorShutdownNow();
}

static void executorShutdownNow() {
Copy link
Member

Choose a reason for hiding this comment

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

Just inline or private static?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just inline or private static?

private static, I have updated,thanks @tisonkun

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate a bit why we need to explicitly shutdown the executor? If the process exits, all tasks just halted.

I think it's more elegant to actively close, control the shutdown wait time,
If the main thread exits, the thread may run incomplete @tisonkun

@StevenLuMT StevenLuMT force-pushed the master_executorShutdown branch from 91b5709 to 1318bfe Compare March 27, 2023 06:54
tisonkun
tisonkun requested changes Mar 27, 2023
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

#19926 (review)

@StevenLuMT Here is a dangling question. I don't think it's a blocker but could you please give it an answer?

@StevenLuMT
Copy link
Member Author

#19926 (review)

@StevenLuMT Here is a dangling question. I don't think it's a blocker but could you please give it an answer?

finished

tisonkun
tisonkun approved these changes Mar 27, 2023
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thanks for your explanation! SGTM.

@AnonHxy AnonHxy added this to the 3.0.0 milestone Mar 28, 2023
@tisonkun
Copy link
Member

@StevenLuMT it seems test failed steadily. Please check locally:

  Error:  Tests run: 25, Failures: 1, Errors: 0, Skipped: 21, Time elapsed: 88.413 s <<< FAILURE! - in org.apache.pulsar.testclient.PerformanceProducerTest
  Error:  testMaxOutstanding(org.apache.pulsar.testclient.PerformanceProducerTest)  Time elapsed: 10.105 s  <<< FAILURE!
  org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a org.apache.pulsar.testclient.PerformanceProducerTest expected object to not be null within 10 seconds.

@StevenLuMT StevenLuMT force-pushed the master_executorShutdown branch from 1318bfe to 388281d Compare March 28, 2023 07:40
@StevenLuMT StevenLuMT changed the title [fix][test] final shutdown executor in main [fix][test] final shutdown executor when the class(PerformanceProducer) is destroyed Mar 28, 2023
@StevenLuMT
Copy link
Member Author

/pulsarbot run-failure-checks

@StevenLuMT StevenLuMT requested a review from tisonkun March 29, 2023 02:32
tisonkun
tisonkun approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @StevenLuMT!

Merging...

@tisonkun tisonkun merged commit d877467 into apache:master Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tisonkun tisonkun tisonkun approved these changes

@AnonHxy AnonHxy AnonHxy approved these changes

@HQebupt HQebupt HQebupt approved these changes

Assignees

@StevenLuMT StevenLuMT

Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@StevenLuMT @tisonkun @HQebupt @AnonHxy