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

ZOOKEEPER-3839: ReconfigBackupTest Remove getFileContent #1360

Closed
wants to merge 6 commits into from
Closed

ZOOKEEPER-3839: ReconfigBackupTest Remove getFileContent #1360

wants to merge 6 commits into from

Conversation

belugabehr
Copy link
Contributor

No description provided.

nkalmar
nkalmar approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@nkalmar nkalmar left a comment

Choose a reason for hiding this comment

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

Nice improvement! +1

ctubbsii
ctubbsii suggested changes May 21, 2020
View reviewed changes
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java Outdated
@@ -93,7 +83,7 @@ public void testBackupStatic() throws Exception {
mt[i] = new MainThread(i, clientPorts[i], currentQuorumCfgSection, false);
// check that a dynamic configuration file doesn't exist
assertNull("static file backup shouldn't exist before bootup", mt[i].getFileByName("zoo.cfg.bak"));
staticFileContent[i] = getFileContent(mt[i].confFile);
staticFileContent[i] = FileUtils.readFileToString(mt[i].confFile, StandardCharsets.UTF_8);
Copy link
Member

Choose a reason for hiding this comment

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

You seem to have a lot of little open PRs for the same action in different parts of the code. It is making reviewing a bit redundant. See my comments on your other PR with my suggestions to use static imports for UTF_8 for readability, and the Java-native options for reading files without needing to use Commons.

@belugabehr
Copy link
Contributor Author

@ctubbsii @nkalmar Another review please :)

When you take a look at it, one of the things to review is that the previous implementation used a Scanner to read line-by-line (stripping newlines) and then adding newlines back on. That's all to say, previously it didn't just read the raw files, it did some small amount of parsing. Now that I am reading just the raw bytes (better that way anyway), I had to make a few modifications to get the tests working without changing the test logic itself.

eolivelli
eolivelli approved these changes Nov 10, 2020
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

Thanks

ctubbsii
ctubbsii approved these changes Nov 10, 2020
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

LGTM

ztzg
ztzg approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@ztzg ztzg left a comment

Choose a reason for hiding this comment

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

LGTM.

@ztzg ztzg closed this in 33bf364 Nov 12, 2020
@ztzg
Copy link
Contributor

ztzg commented Nov 12, 2020

Merged into master. Thank you, @belugabehr!

RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
@belugabehr @RokLenarcic
ZOOKEEPER-3839: ReconfigBackupTest Remove getFileContent
9ada4ac 
Author: David Mollitor <dmollitor@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Enrico Olivelli <eolivelli@apache.org>, Christopher Tubbs <ctubbsii@apache.org>, Damien Diederen <ddiederen@apache.org>

Closes apache#1360 from belugabehr/ZOOKEEPER-3839
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
@belugabehr @RokLenarcic
ZOOKEEPER-3839: ReconfigBackupTest Remove getFileContent
7442197 
Author: David Mollitor <dmollitor@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Enrico Olivelli <eolivelli@apache.org>, Christopher Tubbs <ctubbsii@apache.org>, Damien Diederen <ddiederen@apache.org>

Closes apache#1360 from belugabehr/ZOOKEEPER-3839
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
@belugabehr @RokLenarcic
ZOOKEEPER-3839: ReconfigBackupTest Remove getFileContent
987aeaf 
Author: David Mollitor <dmollitor@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Enrico Olivelli <eolivelli@apache.org>, Christopher Tubbs <ctubbsii@apache.org>, Damien Diederen <ddiederen@apache.org>

Closes apache#1360 from belugabehr/ZOOKEEPER-3839
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
@belugabehr @RokLenarcic
ZOOKEEPER-3839: ReconfigBackupTest Remove getFileContent
ff42622 
Author: David Mollitor <dmollitor@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Enrico Olivelli <eolivelli@apache.org>, Christopher Tubbs <ctubbsii@apache.org>, Damien Diederen <ddiederen@apache.org>

Closes apache#1360 from belugabehr/ZOOKEEPER-3839
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli approved these changes

@nkalmar nkalmar nkalmar approved these changes

@ctubbsii ctubbsii ctubbsii approved these changes

@ztzg ztzg ztzg 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.
5 participants
@belugabehr @ztzg @ctubbsii @nkalmar @eolivelli