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

[pulsar-admin] add read-timeout to blocked admin-request #4484

Merged
merged 2 commits into from
Jun 13, 2019
Merged

[pulsar-admin] add read-timeout to blocked admin-request #4484

merged 2 commits into from
Jun 13, 2019

Conversation

rdhabalia
Copy link
Contributor

Motivation

It fixes #4447 and unblocks broker's admin thread and avoids possible deadlock.

@rdhabalia rdhabalia added this to the 2.4.0 milestone Jun 5, 2019
@rdhabalia rdhabalia self-assigned this Jun 5, 2019
@rdhabalia rdhabalia mentioned this pull request Jun 5, 2019
Closed
@rdhabalia
Copy link
Contributor Author

rerun java8 tests

nkurihar
nkurihar reviewed Jun 6, 2019
View reviewed changes
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/PulsarAdmin.java
this.worker = new WorkerImpl(root, auth);
this.schemas = new SchemasImpl(root, auth);
this.bookies = new BookiesImpl(root, auth);
long readTimeoutMs = readTimeoutUnit.toMillis(this.readTimeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't found the line which sets readTimeout in https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java , so readTimeout seems to be set to the default value (60 seconds) for Brokers.

pulsar/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/PulsarAdmin.java

Line 72 in b5d779d

public static final int DEFAULT_READ_TIMEOUT_SECONDS = 60;

Can you make it configurable in broker.conf?
(I feel 60 seconds is too large for Brokers)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I will fix it.

sijie
sijie requested changes Jun 7, 2019
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

I see some places are using connectTimeoutMs and some places are using readTimeoutMs. Can we be consistent across all the places?

@sijie
Copy link
Member

sijie commented Jun 11, 2019

ping @rdhabalia

@rdhabalia
Copy link
Contributor Author

@nkurihar most of the admin-sync call makes blocking zk call so, configured admin-readtimeout as zkOperationTimeout. Please let me know if we have different thought on it.

@sijie

I see some places are using connectTimeoutMs and some places are using readTimeoutMs. Can we be consistent across all the places?

yes, I have fixed it.

@rdhabalia
Copy link
Contributor Author

rerun java8 tests

@nkurihar
Copy link
Contributor

nkurihar commented Jun 12, 2019 •
edited
Loading

@rdhabalia
Thank you for making readTimeout configurable!

minor comment:
How about adding a comment to zooKeeperOperationTimeoutSeconds in broker.conf to explain that this value is also used as an admin-readtimeout in Brokers?

pulsar/conf/broker.conf

Lines 64 to 65 in a195221

# ZooKeeper operation timeout in seconds
zooKeeperOperationTimeoutSeconds=30

@nkurihar
Copy link
Contributor

rerun java8 tests

@codelipenghui
Copy link
Contributor

@rdhabalia do we need this issue for 2.4.0? or can we move it to 2.5.0?

@rdhabalia
Copy link
Contributor Author

yes, it's ready to merge.

@codelipenghui
Copy link
Contributor

@sijie Please help review again.

@sijie
Copy link
Member

sijie commented Jun 13, 2019

@nkurihar can you review and merge it?

nkurihar
nkurihar approved these changes Jun 13, 2019
View reviewed changes
Copy link
Contributor

@nkurihar nkurihar left a comment

Choose a reason for hiding this comment

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

LGTM

@nkurihar nkurihar merged commit 46a068b into apache:master Jun 13, 2019
@massakam massakam mentioned this pull request Jul 18, 2019
Closed
@k2la k2la mentioned this pull request Jul 19, 2019
Merged
@massakam massakam mentioned this pull request Jul 25, 2019
Merged
@sijie sijie mentioned this pull request Dec 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkurihar nkurihar nkurihar approved these changes

@sijie sijie sijie approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@jiazhai jiazhai Awaiting requested review from jiazhai

@jai1 jai1 Awaiting requested review from jai1

Assignees

@rdhabalia rdhabalia

Labels
area/broker area/cli
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

Broker gets stuck when getting partitioned stats
4 participants
@rdhabalia @sijie @nkurihar @codelipenghui