Skip to content

ZOOKEEPER-4269: acceptedEpoch.tmp rename failure will cause server startup error#1664

Closed
arshadmohammad wants to merge 3 commits intoapache:masterfrom
arshadmohammad:ZOOKEEPER-4269-master
Closed

ZOOKEEPER-4269: acceptedEpoch.tmp rename failure will cause server startup error#1664
arshadmohammad wants to merge 3 commits intoapache:masterfrom
arshadmohammad:ZOOKEEPER-4269-master

Conversation

@arshadmohammad
Copy link
Copy Markdown
Contributor

Using accepted epoch from acceptedEpoch.tmp if it is available

…artup error

Using accepted epoch from acceptedEpoch.tmp if it is available
Copy link
Copy Markdown
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.

good idea.
overall the change looks good to me

it is possible to add a test case ?
probably it is better to have more eyes on this patch
tagging @hanm @symat @anmolnar

@arshadmohammad
Copy link
Copy Markdown
Contributor Author

Added test case

@arshadmohammad arshadmohammad requested review from anmolnar and removed request for anmolnar March 30, 2021 17:11
Copy link
Copy Markdown
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.

I must admit that I'm not quite on board with the idea of reading from .tmp files; this somehow goes against the idea of writing the file "atomically." I understand that these two small files are somewhat troublesome, though.

Independently of my feelings, the test should be updated to use the new JUnit 5 API in this branch.

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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.

Okay, LGTM. (Still a bit queasy about these .tmp files.)

@asfgit asfgit closed this in cdddda4 Mar 31, 2021
asfgit pushed a commit that referenced this pull request Mar 31, 2021
…artup error

Using accepted epoch from acceptedEpoch.tmp if it is available

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>,Damien Diederen <dd@crosstwine.com>

Closes #1664 from arshadmohammad/ZOOKEEPER-4269-master and squashes the following commits:

d8ae084 [Mohammad Arshad] Handled review comments
a2cc9a5 [Mohammad Arshad] Added test cases
b56837e [Mohammad Arshad] ZOOKEEPER-4269: acceptedEpoch.tmp rename failure will cause server startup error Using accepted epoch from acceptedEpoch.tmp if it is available

(cherry picked from commit cdddda4)
Signed-off-by: Mohammad Arshad <arshad@apache.org>
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.

3 participants