Skip to content

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

Merged
tisonkun merged 1 commit into
apache:masterfrom
StevenLuMT:master_executorShutdown
Mar 29, 2023
Merged

[fix][test] final shutdown executor when the class(PerformanceProducer) is destroyed#19926
tisonkun merged 1 commit into
apache:masterfrom
StevenLuMT:master_executorShutdown

Conversation

@StevenLuMT

@StevenLuMT StevenLuMT commented Mar 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

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.

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

static void executorShutdownNow() {

@tisonkun tisonkun Mar 25, 2023

Copy link
Copy Markdown
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?

@StevenLuMT StevenLuMT Mar 27, 2023

Copy link
Copy Markdown
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

@StevenLuMT StevenLuMT Mar 27, 2023

Copy link
Copy Markdown
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

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

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

StevenLuMT commented Mar 27, 2023

Copy link
Copy Markdown
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

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

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

tisonkun commented Mar 28, 2023

Copy link
Copy Markdown
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

StevenLuMT commented Mar 28, 2023

Copy link
Copy Markdown
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

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

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

@HQebupt HQebupt HQebupt approved these changes

@AnonHxy AnonHxy AnonHxy approved these changes

@tisonkun tisonkun tisonkun 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