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.

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

ZOOKEEPER-2825: 1. Remove unnecessary import; 2. `contains` instead of `indexOf > -1` for more readable; 3. Standardize `StringBuilder#append` usage for CLIENT module #297

Closed
wants to merge 1 commit into from

Conversation

@asdf2014
Copy link
Member

commented Jun 29, 2017

  • Remove unnecessary import;
  • contains instead of indexOf > -1 for more readable;
  • Standardize StringBuilder#append usage for CLIENT module

@asdf2014 asdf2014 force-pushed the asdf2014:ZOOKEEPER-2825 branch from 91ef299 to 4386cbb Jun 29, 2017

@asdf2014 asdf2014 changed the title ZOOKEEPER-2825: 1. Remove unnecessary import; 2. `contains` instead of `indexOf > -1` for more readable; 3. Standardize `StringBuilder#append` usage for CLI module ZOOKEEPER-2825: 1. Remove unnecessary import; 2. `contains` instead of `indexOf > -1` for more readable; 3. Standardize `StringBuilder#append` usage for CLIENT module Jun 29, 2017

@asdf2014 asdf2014 force-pushed the asdf2014:ZOOKEEPER-2825 branch from 4386cbb to c627e41 Jun 29, 2017

@@ -590,14 +590,13 @@ public boolean clientTunneledAuthenticationInProgress() {
// authentication is either in progress, successful, or failed.

// 1. Authentication hasn't finished yet: we must wait for it to do so.
if ((isComplete() == false) &&
(isFailed() == false)) {
if ((!isComplete()) && (!isFailed())) {
return true;
}

// 2. SASL authentication has succeeded or failed..
if (isComplete() || isFailed()) {

This comment has been minimized.

Copy link
@shralex

shralex Jun 29, 2017

Contributor

wouldn't this always be true because of the previous if condition ?

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Jun 29, 2017

Author Member

Hi, @shralex. Yep, if we get the value of !isComplete() is true, then we already know the authentication has not yet been completed, so it should return true for the clientTunneledAuthenticationInProgress method directly.

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Jun 29, 2017

Author Member

Maybe i understand what you mean, if not_complete && not_failed (Line: 593) is FALSE means complete || non_failed and complete || failed and non_complete || failed is TRUE, then the isComplete() || isFailed() (Line: 599) will always be TRUE.

@asdf2014 asdf2014 force-pushed the asdf2014:ZOOKEEPER-2825 branch from 05d02f0 to 49e9b01 Jun 29, 2017

@shralex

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2017

LGTM

if (gotLastPacket == false) {
// ..but still in progress, because there is a final SASL
// message from server which must be received.
if (!gotLastPacket) {

This comment has been minimized.

Copy link
@afine

afine Jun 29, 2017

Contributor

this appears to be a logic change. Was that intended?

This comment has been minimized.

Copy link
@afine

afine Jun 29, 2017

Contributor

nvm, did not see the code above. lgtm

@afine

afine approved these changes Jun 29, 2017

Copy link
Contributor

left a comment

+1

@anmolnar

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

@asdf2014 Changes in the patch look like still valuable. Would you mind rebasing it?

@asdf2014

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

@anmolnar Sure, I will rebase this.

* Remove unnecessary import
* `contains` instead of `indexOf > -1` for more readable

* Standardize `StringBuilder#append` usage for CLIENT module

* Remove unnecessary `!isFailed()` for `clientTunneledAuthenticationInProgress` method

* Remove unnecessary `isComplete() || isFailed()` for `clientTunneledAuthenticationInProgress` method

@asdf2014 asdf2014 force-pushed the asdf2014:ZOOKEEPER-2825 branch from 22202ae to d1a2536 Jan 31, 2019

@asdf2014

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

@anmolnar Done.

@asfgit asfgit closed this in cbc63db Jan 31, 2019

asfgit pushed a commit that referenced this pull request Jan 31, 2019

ZOOKEEPER-2825: 1. Remove unnecessary import; 2. `contains` instead o…
…f `indexOf > -1` for more readable; 3. Standardize `StringBuilder#append` usage for CLIENT module

* Remove unnecessary import;
* `contains` instead of `indexOf > -1` for more readable;
* Standardize `StringBuilder#append` usage for CLIENT module

Author: asdf2014 <benedictjin2016@gmail.com>

Reviewers: afine@apache.org, andor@apache.org

Closes #297 from asdf2014/ZOOKEEPER-2825

(cherry picked from commit cbc63db)
Signed-off-by: Andor Molnar <andor@apache.org>
@anmolnar

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

Merged to 3.5 and master branches. Thanks @asdf2014 !

@asdf2014

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

@anmolnar You are welcome.

@asdf2014 asdf2014 deleted the asdf2014:ZOOKEEPER-2825 branch Feb 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.