Skip to content

[fix][client]Fix scheduledExecutorProvider not shutdown #17527

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
Merged
nicoloboschi merged 2 commits into from
Sep 13, 2022
Merged

[fix][client]Fix scheduledExecutorProvider not shutdown #17527

nicoloboschi merged 2 commits into from
Sep 13, 2022

Conversation

@AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Sep 7, 2022

Motivation

  • Fix org.apache.pulsar.client.impl.PulsarClientImpl#scheduledExecutorProvider not shutdown

Modifications

  • Shut down scheduledExecutorProvider

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc-not-needed

@AnonHxy AnonHxy self-assigned this Sep 7, 2022
@merlimat merlimat added this to the 2.11.0 milestone Sep 7, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 7, 2022
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 8, 2022

/pulsarbot run-failure-checks

@Technoboy- Technoboy- force-pushed the fix_scheduledExecutorProvider_not_shutdown branch from bcc14ca to 5d41763 Compare September 8, 2022 02:35
Jason918
Jason918 approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

Good catch.

@Jason918
Copy link
Contributor

Jason918 commented Sep 8, 2022

@Technoboy- @codelipenghui This PR fixed the bug introduced by #16334, which is included in 2.11.0. Can we also add this in 2.11.0?

nicoloboschi
nicoloboschi requested changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

It looks like the null-check is still needed

Caused by: java.lang.NullPointerException: Cannot invoke "org.apache.pulsar.client.util.ScheduledExecutorProvider.isShutdown()" because "this.scheduledExecutorProvider" is null
at org.apache.pulsar.client.impl.PulsarClientImpl.shutdownExecutors(PulsarClientImpl.java:895)
at org.apache.pulsar.client.impl.PulsarClientImpl.shutdown(PulsarClientImpl.java:833)
at org.apache.pulsar.client.impl.PulsarClientImpl.(PulsarClientImpl.java:234)
at org.apache.pulsar.client.impl.PulsarClientImpl.(PulsarClientImpl.java:154)
at org.apache.pulsar.client.impl.ClientBuilderImpl.build(ClientBuilderImpl.java:63)
at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.createNewPulsarClient(MockedPulsarServiceBaseTest.java:183)
at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.replacePulsarClient(MockedPulsarServiceBaseTest.java:190)
at org.apache.pulsar.client.impl.ClientWithSocks5ProxyTest.testSetErrorProxyAddress(ClientWithSocks5ProxyTest.java:167)

@AnonHxy AnonHxy force-pushed the fix_scheduledExecutorProvider_not_shutdown branch from 5d41763 to da29c84 Compare September 8, 2022 08:20
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 8, 2022

/pulsarbot run-failure-checks

1 similar comment
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 8, 2022

/pulsarbot run-failure-checks

@Technoboy- Technoboy- closed this Sep 9, 2022
@Technoboy- Technoboy- reopened this Sep 9, 2022
@Technoboy-
Copy link
Contributor

Technoboy- commented Sep 9, 2022

@Technoboy- @codelipenghui This PR fixed the bug introduced by #16334, which is included in 2.11.0. Can we also add this in 2.11.0?

yes, sure

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 9, 2022

It looks like the null-check is still needed

Fixed. @nicoloboschi PTAL

@Jason918
Copy link
Contributor

Jason918 commented Sep 12, 2022

@nicoloboschi PTAL. This is a release blocker for 2.10.2.

@hezhangjian
Copy link
Member

hezhangjian commented Sep 12, 2022

Nice catch!

@nicoloboschi nicoloboschi merged commit a23963d into apache:master Sep 13, 2022
nicoloboschi pushed a commit that referenced this pull request Sep 13, 2022
@AnonHxy @nicoloboschi
[fix][client]Fix scheduledExecutorProvider not shutdown (#17527)
913b74b 
* Fix scheduledExecutorProvider not shutdown

* Fix check null

(cherry picked from commit a23963d)
nicoloboschi pushed a commit that referenced this pull request Sep 13, 2022
@AnonHxy @nicoloboschi
[fix][client]Fix scheduledExecutorProvider not shutdown (#17527)
296f0f0 
* Fix scheduledExecutorProvider not shutdown

* Fix check null

(cherry picked from commit a23963d)
tisonkun pushed a commit to tisonkun/pulsar that referenced this pull request Sep 14, 2022
@AnonHxy @tisonkun
[fix][client]Fix scheduledExecutorProvider not shutdown (apache#17527)
b8f5224 
* Fix scheduledExecutorProvider not shutdown

* Fix check null
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Sep 16, 2022
@AnonHxy @nicoloboschi
[fix][client]Fix scheduledExecutorProvider not shutdown (apache#17527)
6bc7cf8 
* Fix scheduledExecutorProvider not shutdown

* Fix check null

(cherry picked from commit a23963d)
(cherry picked from commit 296f0f0)
congbobo184 pushed a commit that referenced this pull request Nov 14, 2022
@AnonHxy
[fix][client]Fix scheduledExecutorProvider not shutdown (#17527)
4c3e56b 
* Fix scheduledExecutorProvider not shutdown

* Fix check null

(cherry picked from commit a23963d)
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 14, 2022
congbobo184 pushed a commit that referenced this pull request Dec 7, 2022
@AnonHxy
[fix][client]Fix scheduledExecutorProvider not shutdown (#17527)
5a67cfc 
* Fix scheduledExecutorProvider not shutdown

* Fix check null

(cherry picked from commit a23963d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merlimat merlimat merlimat approved these changes

@Jason918 Jason918 Jason918 approved these changes

@Technoboy- Technoboy- Technoboy- approved these changes

@hezhangjian hezhangjian hezhangjian approved these changes

@nodece nodece nodece approved these changes

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

Assignees

@AnonHxy AnonHxy

Labels

area/client cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs release/2.9.4 release/2.10.2 release/2.11.0 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

9 participants

@AnonHxy @Jason918 @Technoboy- @hezhangjian @merlimat @nodece @nicoloboschi @tisonkun @congbobo184