Skip to content

ZOOKEEPER-3196. Maintain the configuration will be used by server stabilizer.It can be overridden based on the server type and the server system internals. #712

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
Open
tumativ wants to merge 5 commits into
base: master
Choose a base branch
from
Open

ZOOKEEPER-3196. Maintain the configuration will be used by server stabilizer.It can be overridden based on the server type and the server system internals. #712

tumativ wants to merge 5 commits into from
+57 −30

Conversation

@tumativ
Copy link

@tumativ tumativ commented Nov 21, 2018 •
edited
Loading

https://issues.apache.org/jira/browse/ZOOKEEPER-3196

Maintain the configuration will be used by server stabilizer. It can be overridden based on the server type and the server system internals.

-Avoid calculating the globalOutstandingLimit for every request as it is not going to be changed for every request.

-we are reading globalOutstandingLimit from the system property and parsing the value at every call of shouldThrottle. So it can be taken from config. It will act as the cache.

anmolnar
anmolnar reviewed Nov 21, 2018
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

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

Interesting approach and worth to think about. This one duplicates my revert: #711
I leave the decision to @lvfangmin and @hanm as they're the original committers.

@anmolnar anmolnar mentioned this pull request Nov 21, 2018
Closed
@eolivelli
Copy link
Contributor

eolivelli commented Nov 21, 2018

Can we add in the descriptoin the original problem fro findbugs ?
It is not clear what we are going to fix.

eolivelli
eolivelli requested changes Nov 21, 2018
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

left some questions and comments.
Thank you for working on this

@anmolnar
Copy link
Contributor

anmolnar commented Nov 21, 2018

@tumativ Given that this is not a straight revert of the failing patch, would you please create a Jira and explain the reasoning in that?

@tumativ
Copy link
Author

tumativ commented Nov 21, 2018

The build is green. It addressed the issues.

@tumativ
Copy link
Author

tumativ commented Nov 21, 2018

https://issues.apache.org/jira/browse/ZOOKEEPER-3196

@tumativ tumativ changed the title Master FindBug issues fix Maintain the configuration will be used by server stabilizer and override based on the server type and the server system internals. Nov 21, 2018
@tumativ tumativ changed the title Maintain the configuration will be used by server stabilizer and override based on the server type and the server system internals. Maintain the configuration will be used by server stabilizer.It can be overridden based on the server type and the server system internals. Nov 21, 2018
anmolnar
anmolnar approved these changes Dec 5, 2018
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

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

+1 lgtm

@anmolnar
Copy link
Contributor

anmolnar commented May 7, 2019

@tumativ Please rebase this patch if you're still working on it.
Currently the implementation is flooding the logs with messages like this:

2019-05-07 16:03:58,679 [myid:1] - INFO  [NIOWorkerThread-5:FollowerZooKeeperServer@137] - Override zookeeper.globalOutstandingLimit to 500
2019-05-07 16:03:58,682 [myid:1] - INFO  [CommitProcWorkThread-2:FollowerZooKeeperServer@137] - Override zookeeper.globalOutstandingLimit to 500
2019-05-07 16:03:58,685 [myid:1] - INFO  [NIOWorkerThread-8:FollowerZooKeeperServer@137] - Override zookeeper.globalOutstandingLimit to 500
2019-05-07 16:03:58,687 [myid:1] - INFO  [CommitProcWorkThread-2:FollowerZooKeeperServer@137] - Override zookeeper.globalOutstandingLimit to 500
2019-05-07 16:03:58,691 [myid:1] - INFO  [NIOWorkerThread-11:FollowerZooKeeperServer@137] - Override zookeeper.globalOutstandingLimit to 500
2019-05-07 16:03:58,693 [myid:1] - INFO  [CommitProcWorkThread-2:FollowerZooKeeperServer@137] - Override zookeeper.globalOutstandingLimit to 500
2019-05-07 16:03:58,696 [myid:1] - INFO  [NIOWorkerThread-14:FollowerZooKeeperServer@137] - Override zookeeper.globalOutstandingLimit to 500
2019-05-07 16:03:58,698 [myid:1] - INFO  [CommitProcWorkThread-2:FollowerZooKeeperServer@137] - Override zookeeper.globalOutstandingLimit to 500

I'd like to commit this.

@anmolnar
Copy link
Contributor

anmolnar commented May 7, 2019

@eolivelli @lvfangmin PTAL.

eolivelli
eolivelli approved these changes May 7, 2019
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm.

I wonder if we could add any test, the fact that we are using a different value for the Follower will have a real effect so we should ensure that this new behaviour won't change in the future.
Just a single UT will be enough

lvfangmin
lvfangmin reviewed Jun 7, 2019
View reviewed changes
zookeeper-server/src/main/java/org/apache/zookeeper/common/ZookeeperServerStabilizerConfig.java
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class ZookeeperServerStabilizerConfig {
Copy link
Contributor

@lvfangmin lvfangmin Jun 7, 2019

Choose a reason for hiding this comment

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

Can you add some simple description for this class.

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
public class ZooKeeperServer implements SessionExpirer, ServerStats.Provider {
protected static final Logger LOG;

public static final String GLOBAL_OUTSTANDING_LIMIT = "zookeeper.globalOutstandingLimit";
Copy link
Contributor

@lvfangmin lvfangmin Jun 7, 2019

Choose a reason for hiding this comment

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

Should we remove this one since it's being included in ZookeeperServerStabilizerConfig?

@anmolnar
Copy link
Contributor

anmolnar commented Jun 14, 2019

@tumativ Sorry for the delay, are u still working on this patch?

@anmolnar anmolnar changed the title Maintain the configuration will be used by server stabilizer.It can be overridden based on the server type and the server system internals. ZOOKEEPER-3196. Maintain the configuration will be used by server stabilizer.It can be overridden based on the server type and the server system internals. Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lvfangmin lvfangmin lvfangmin left review comments

@anmolnar anmolnar anmolnar approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tumativ @eolivelli @anmolnar @lvfangmin