Skip to content

ZOOKEEPER-4257: learner.asyncSending and learner.closeSocketAsync should be configurable in zoo.cfg #1645

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
Closed
ayushmantri wants to merge 2 commits into from
Closed

ZOOKEEPER-4257: learner.asyncSending and learner.closeSocketAsync should be configurable in zoo.cfg #1645

ayushmantri wants to merge 2 commits into from

Conversation

@ayushmantri
Copy link
Contributor

@ayushmantri ayushmantri commented Mar 17, 2021

Properties learner.asyncSending , learner.closeSocketAsync and leader.closeSocketAsync made configurable in zoo.cfg

@arshadmohammad
Copy link
Contributor

arshadmohammad commented Mar 22, 2021

Lets wait for the 3.7.0 releases. We have to update the change for branch-3.7. Have to change the property in backward compatible way.

@functioner
Copy link

functioner commented Mar 23, 2021

@arshadmohammad @ayushmantri Thanks for the commit. This configuration is important and helpful for #1582, and I appreciate it. However, the description for learner.asyncSending you provide in docs is not as detailed as #1582 I provided. I think providing more details about this configuration is helpful for users and developers. Do you agree? And we can improve the description together if you want.

@arshadmohammad
Copy link
Contributor

arshadmohammad commented Mar 23, 2021

sure @functioner
@ayushmantri please take the description from #1582, lets merge this in master branch, we can merge in branch-3.7 later.

@arshadmohammad
Copy link
Contributor

arshadmohammad commented Mar 23, 2021

@functioner please raise PR to backport ZOOKEEPER-3575 in branch-3.6

eolivelli
eolivelli requested changes Mar 23, 2021
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.

It looks like that this is a breaking change.

Do we really need to change the name of the property?
What about adding a compatibility layer (reading the old and the new name)

@arshadmohammad
Copy link
Contributor

arshadmohammad commented Mar 23, 2021 •
edited
Loading

Do we really need to change the name of the property?

yes, it is required to make it configurable in configuration file zoo.cfg. Currently it is not configurable in zoo.cfg. it must be configured as java system property in some shell script file. For example, before this PR change, if I configure learner.asyncSending=true in zoo.cfg It will not be effective as in ZooKeeper before use this configured property is converted to zookeeper.learner.asyncSending.

@arshadmohammad
Copy link
Contributor

arshadmohammad commented Mar 23, 2021

What about adding a compatibility layer (reading the old and the new name)

yes, compatibility layer can be added. Only thing is now there will three properties for the same thing
java system properties

  • learner.asyncSending
  • zookeeper.learner.asyncSending

and normal configuration property, configurable in zoo.cfg

  • learner.asyncSending

@functioner
Copy link

functioner commented Mar 24, 2021

@functioner please raise PR to backport ZOOKEEPER-3575 in branch-3.6

@arshadmohammad I raised the PR in #1653.

@ayushmantri
ZOOKEEPER-4257: learner.asyncSending and learner.closeSocketAsync sho…
8ff5523 
…uld be configurable in zoo.cfg

                property name change done in backward compatible way
@ayushmantri
Copy link
Contributor Author

ayushmantri commented Mar 25, 2021

handled property name change in backward compatible way

arshadmohammad
arshadmohammad approved these changes Mar 27, 2021
View reviewed changes
Copy link
Contributor

@arshadmohammad arshadmohammad left a comment

Choose a reason for hiding this comment

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

LGTM +1

eolivelli
eolivelli approved these changes Mar 27, 2021
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.

Great

+1

@asfgit asfgit closed this in acbfb2d Mar 28, 2021
asfgit pushed a commit that referenced this pull request Mar 28, 2021
@ayushmantri @arshadmohammad
ZOOKEEPER-4257: learner.asyncSending, learner.closeSocketAsync and le…
e2f3108 
…ader.closeSocketAsync should be configurable in zoo.cfg

Properties learner.asyncSending , learner.closeSocketAsync  and leader.closeSocketAsync  made configurable in zoo.cfg

Author: Ayush Mantri <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Mohammad Arshad <[email protected]>

Closes #1645 from ayushmantri/ZK-4257 and squashes the following commits:

8ff5523 [Ayush Mantri] ZOOKEEPER-4257: learner.asyncSending and learner.closeSocketAsync should be configurable in zoo.cfg                 property name change done in backward compatible way
5929312 [Ayush Mantri] ZOOKEEPER-4257: learner.asyncSending and learner.closeSocketAsync should be configurable in zoo.cfg

(cherry picked from commit acbfb2d)
Signed-off-by: Mohammad Arshad <[email protected]>
@arshadmohammad arshadmohammad mentioned this pull request Mar 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@ayushmantri @arshadmohammad @functioner @eolivelli