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

Possible fix and unit test. Fixes #961 #1048

Merged
merged 3 commits into from Mar 25, 2019

Conversation

@milleruntime
Copy link
Contributor

commented Mar 21, 2019

One possible fix for this issue. This just ignores the failed file. AFAIK we aren't actually doing anything with this failed file so it could just be removed. But maybe that be done for 2.0

@ctubbsii
Copy link
Member

left a comment

Should we at least log a debug message?

@milleruntime

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Should we at least log a debug message?

It already logs the original error. I am not sure how much value another log message adds...

@Test
public void testFailed() throws Exception {
Path manyMaps = new Path("file://" + root.getRoot().getAbsolutePath() + "/manyMaps");
fs.create(new Path(manyMaps, SortedLogState.FAILED.getMarker())).close();

This comment has been minimized.

Copy link
@keith-turner

keith-turner Mar 21, 2019

Contributor

Will other test see this file?

This comment has been minimized.

Copy link
@milleruntime

milleruntime Mar 21, 2019

Author Contributor

That is a good question. I will check

This comment has been minimized.

Copy link
@milleruntime

milleruntime Mar 25, 2019

Author Contributor

I added a delete at the end of the Test

@@ -127,6 +127,9 @@ public RecoveryLogReader(VolumeManager fs, Path directory, LogFileKey start, Log
foundFinish = true;
continue;
}
if (SortedLogState.FAILED.getMarker().equals(child.getPath().getName())) {

This comment has been minimized.

Copy link
@keith-turner

keith-turner Mar 21, 2019

Contributor

Could add this condition as an OR to the first if stmt in the for loop.

This comment has been minimized.

Copy link
@milleruntime

milleruntime Mar 22, 2019

Author Contributor

I was thinking this is easier to remove, if that's what we plan on for 2.0.

@milleruntime milleruntime merged commit 995a952 into apache:1.9 Mar 25, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@milleruntime milleruntime deleted the milleruntime:log-recovery-fix branch Mar 25, 2019

@ctubbsii ctubbsii added this to Done in 1.9.3 Jun 14, 2019

@ctubbsii ctubbsii added this to Done in 2.0.0 Jun 14, 2019

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