Skip to content

Conversation

@brkyvz
Copy link
Contributor

@brkyvz brkyvz commented Nov 8, 2016

What changes were proposed in this pull request?

If the rename operation in the state store fails (fs.rename returns false), the StateStore should throw an exception and have the task retry. Currently if renames fail, nothing happens during execution immediately. However, you will observe that snapshot operations will fail, and then any attempt at recovery (executor failure / checkpoint recovery) also fails.

How was this patch tested?

Unit test

@brkyvz
Copy link
Contributor Author

brkyvz commented Nov 8, 2016

cc @tdas

@SparkQA
Copy link

SparkQA commented Nov 8, 2016

Test build #68317 has finished for PR 15804 at commit e0c164b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class RenameReturnsFalseFileSystem extends RawLocalFileSystem

val deltaFiles = allFiles.filter { file =>
file.version > snapshotFile.version && file.version <= version
}
}.toList
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the line returns a Stream which is a Scala Iterator like class, therefore the logging below doesn't work

@SparkQA
Copy link

SparkQA commented Nov 8, 2016

Test build #68346 has finished for PR 15804 at commit 98b8850.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.


/** Fake FileSystem to test whether the method `fs.exists` is called during
* `DataSource.resolveRelation`.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong docs.

}
}

test("commit fails when rename fails") {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add the jira number here.

@tdas
Copy link
Contributor

tdas commented Nov 8, 2016

looks good. to minor comments.

@brkyvz
Copy link
Contributor Author

brkyvz commented Nov 8, 2016

@tdas Addressed

@SparkQA
Copy link

SparkQA commented Nov 8, 2016

Test build #68356 has finished for PR 15804 at commit db748dd.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@tdas
Copy link
Contributor

tdas commented Nov 8, 2016

LGTM.

@SparkQA
Copy link

SparkQA commented Nov 8, 2016

Test build #68358 has finished for PR 15804 at commit 73b1ab6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@tdas
Copy link
Contributor

tdas commented Nov 8, 2016

Merge this to master, 2.1 and 2.0

asfgit pushed a commit that referenced this pull request Nov 8, 2016
## What changes were proposed in this pull request?

If the rename operation in the state store fails (`fs.rename` returns `false`), the StateStore should throw an exception and have the task retry. Currently if renames fail, nothing happens during execution immediately. However, you will observe that snapshot operations will fail, and then any attempt at recovery (executor failure / checkpoint recovery) also fails.

## How was this patch tested?

Unit test

Author: Burak Yavuz <brkyvz@gmail.com>

Closes #15804 from brkyvz/rename-state.

(cherry picked from commit 6f7ecb0)
Signed-off-by: Tathagata Das <tathagata.das1565@gmail.com>
asfgit pushed a commit that referenced this pull request Nov 8, 2016
## What changes were proposed in this pull request?

If the rename operation in the state store fails (`fs.rename` returns `false`), the StateStore should throw an exception and have the task retry. Currently if renames fail, nothing happens during execution immediately. However, you will observe that snapshot operations will fail, and then any attempt at recovery (executor failure / checkpoint recovery) also fails.

## How was this patch tested?

Unit test

Author: Burak Yavuz <brkyvz@gmail.com>

Closes #15804 from brkyvz/rename-state.

(cherry picked from commit 6f7ecb0)
Signed-off-by: Tathagata Das <tathagata.das1565@gmail.com>
@asfgit asfgit closed this in 6f7ecb0 Nov 8, 2016
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
## What changes were proposed in this pull request?

If the rename operation in the state store fails (`fs.rename` returns `false`), the StateStore should throw an exception and have the task retry. Currently if renames fail, nothing happens during execution immediately. However, you will observe that snapshot operations will fail, and then any attempt at recovery (executor failure / checkpoint recovery) also fails.

## How was this patch tested?

Unit test

Author: Burak Yavuz <brkyvz@gmail.com>

Closes apache#15804 from brkyvz/rename-state.
@brkyvz brkyvz deleted the rename-state branch February 3, 2019 20:58
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