Skip to content

continue can be replaced with break#553

Closed
xiaoed wants to merge 1 commit intoapache:masterfrom
xiaoed:master
Closed

continue can be replaced with break#553
xiaoed wants to merge 1 commit intoapache:masterfrom
xiaoed:master

Conversation

@xiaoed
Copy link
Copy Markdown

@xiaoed xiaoed commented Jun 27, 2018

FileTxnLog.getLogFiles()
continue can be replaced with break

@nkalmar
Copy link
Copy Markdown
Contributor

nkalmar commented Jun 27, 2018

Thanks @a470577391 , looks good, I think you're right, but please create a jira for this PR on https://issues.apache.org/jira/projects/ZOOKEEPER/
And make sure the jira number is in the commit message.

continue;
break;
}
// the files
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@a470577391 Yes,since the files is already order by zxid asc.
I have two suggestions personally if you want to open a new JIRA:
1: rename the method to a explicit one, getLogFiles is ambiguous
2. use Java8 lambda to rewrite the method elegantly,stream, filter, max will be useful.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for your suggestion.

  1. i'm going to try.
  2. i think use Java8 may be unfriendly to the lower version.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

  1. ZooKeeper runs in Java, release 1.8 or greater 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@a470577391 ZooKeeper 3.4.x runs on Java7, version 3.5.x and 3.6.x are not stable yet, they use Java8.

@maoling
Copy link
Copy Markdown
Member

maoling commented Jun 28, 2018

IMHO:
If the file are sorted,can we use binary search ,find the index then subList?
Should we call Util.getZxidFromName twice?
If we rewrite this method ,a unit test is really needed,because the method is very fundamental

@anmolnar
Copy link
Copy Markdown
Contributor

anmolnar commented Jul 4, 2018

@a470577391 Thanks for the contribution.
As others requested already would you please create a Jira for this issue describing what exactly would like to change and why.
Additionally, I doubt this patch is going to target 3.4 branch, so feel free to use Java 8 features.

@maoling
Copy link
Copy Markdown
Member

maoling commented Jul 13, 2018

@a470577391
it's useful and enough to change continue to break,especially when call getLogFiles(logDir.listFiles(), 0)
It's better to create a JIRA issues (sign up JIRA if you don't have an account) to bind this PR to a JIRA-ID
The contributor guide is here

@xiaoed xiaoed closed this Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants