You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celeborn.apache.org by GitBox <gi...@apache.org> on 2022/11/23 04:35:51 UTC
[GitHub] [incubator-celeborn] SzyWilliam opened a new pull request, #996: [CELEBORN-44] Fix snapshot loading error
SzyWilliam opened a new pull request, #996:
URL: https://github.com/apache/incubator-celeborn/pull/996
# Fix snapshot loading error
### What changes were proposed in this pull request?
Refresh snapshot cache when statemachine takes a new snapshot.
### Why are the changes needed?
Currently, `getLatestSnapshot` only returns cached snapshot. However, when StateMachine takes a new snapshot, it will not update cache, so `getLatestSnapshot` cannot get the latest snapshot.
I add the update cache step in the last of `takeSnapshot`.
### How was this patch tested?
Add new unit tests to verify.
/cc @related-reviewer
/assign @main-reviewer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] SzyWilliam commented on a diff in pull request #996: [CELEBORN-44][BUG] StateMachine not update currentSnapshot after takeSnapshot cause getLatestSnapshot return null
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #996:
URL: https://github.com/apache/incubator-celeborn/pull/996#discussion_r1030087597
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/StateMachine.java:
##########
@@ -344,6 +344,7 @@ public long takeSnapshot() {
LOG.warn("Failed to rename snapshot from {} to {}.", tempFile, snapshotFile);
return RaftLog.INVALID_LOG_INDEX;
}
+ storage.loadLatestSnapshot();
Review Comment:
`StateMachine`'s `getLatestSnapshot` is called every turn when `LogAppender` sends `AppendEntries`, see https://github.com/apache/ratis/blob/729d3dce220f352a720cbe38d0762399b6e55f06/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java#L154.
`loadSnapshot` is a heavy operation, since it will scan the file system and uses regular expressions. If we move this method to `getLatestSnapshot`, it maybe a performance problem. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #996: [CELEBORN-44][BUG] StateMachine not update currentSnapshot after takeSnapshot cause getLatestSnapshot return null
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #996:
URL: https://github.com/apache/incubator-celeborn/pull/996#discussion_r1030088105
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/StateMachine.java:
##########
@@ -344,6 +344,7 @@ public long takeSnapshot() {
LOG.warn("Failed to rename snapshot from {} to {}.", tempFile, snapshotFile);
return RaftLog.INVALID_LOG_INDEX;
}
+ storage.loadLatestSnapshot();
Review Comment:
Yea， I have just thought about such a thing too...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] waitinfuture merged pull request #996: [CELEBORN-44][BUG] StateMachine not update currentSnapshot after takeSnapshot cause getLatestSnapshot return null
Posted by GitBox <gi...@apache.org>.
waitinfuture merged PR #996:
URL: https://github.com/apache/incubator-celeborn/pull/996
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #996: [CELEBORN-44] Fix snapshot loading error
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #996:
URL: https://github.com/apache/incubator-celeborn/pull/996#discussion_r1030063591
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/StateMachine.java:
##########
@@ -344,6 +344,7 @@ public long takeSnapshot() {
LOG.warn("Failed to rename snapshot from {} to {}.", tempFile, snapshotFile);
return RaftLog.INVALID_LOG_INDEX;
}
+ storage.loadLatestSnapshot();
Review Comment:
Good catch! GA failed only for spark-3.1, can you re-trigger again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #996: [CELEBORN-44][BUG] StateMachine not update currentSnapshot after takeSnapshot cause getLatestSnapshot return null
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #996:
URL: https://github.com/apache/incubator-celeborn/pull/996#discussion_r1030082104
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/StateMachine.java:
##########
@@ -344,6 +344,7 @@ public long takeSnapshot() {
LOG.warn("Failed to rename snapshot from {} to {}.", tempFile, snapshotFile);
return RaftLog.INVALID_LOG_INDEX;
}
+ storage.loadLatestSnapshot();
Review Comment:
Maybe we should remove this method to `StateMachine`'s `getLatestSnapshot` with a try catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org