You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2021/05/31 09:09:03 UTC
[GitHub] [ratis] guihecheng opened a new pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
guihecheng opened a new pull request #479:
URL: https://github.com/apache/ratis/pull/479
## What changes were proposed in this pull request?
Introduce a '.storage.space.reserved' for storage dir, it is used to prevent RaftServer from failing to take a final snapshot
when raftgroups are removed.
This threshold is only saved for final snapshots and should not be very large (e.g. 100MB),
and it is not for ratis log disks to be shared with other services, so maybe a simple global config is enough.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1377
## How was this patch tested?
a new ut.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r648127289
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -51,6 +51,16 @@ static void setStorageDir(RaftProperties properties, List<File> storageDir) {
setFiles(properties::setFiles, STORAGE_DIR_KEY, storageDir);
}
+ String STORAGE_SPACE_RESERVED_KEY = PREFIX + ".storage.space.reserved";
Review comment:
@szetszwo Ah, yeah, I agree with you, this should better be minRequired space for a ratis storageDir.
So what about name it to '.storage.space.min.required' ?
(Reserved should be for other app on the same disk if there is any.)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #479: RATIS-1377. Ratis min free space for storage dirs.
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #479:
URL: https://github.com/apache/ratis/pull/479
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r643621498
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageDirectoryImpl.java
##########
@@ -150,6 +152,13 @@ StorageState analyzeStorage(boolean toLock) throws IOException {
this.lock(); // lock storage if it exists
}
+ // check enough space
+ if (!hasEnoughSpace()) {
+ LOG.warn("There are not enough space left for directory " + rootPath
+ + "required: " + spaceReserved + " left: " + root.getFreeSpace());
Review comment:
OK.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r648116323
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -51,6 +51,16 @@ static void setStorageDir(RaftProperties properties, List<File> storageDir) {
setFiles(properties::setFiles, STORAGE_DIR_KEY, storageDir);
}
+ String STORAGE_SPACE_RESERVED_KEY = PREFIX + ".storage.space.reserved";
Review comment:
In the change below, it compares only the free spaces but not including the occupied spaces.
```root.getFreeSpace() > spaceReserved;```
The picture actually is
|------------for server to consume---------|-- existing snapshots & log files --|reserved |
So we cannot say that the space is reserved for snapshots. Reserved space is more complicated to implement.
It seems that the amount of the space is the minimum required for starting a server.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r643623331
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -51,6 +51,16 @@ static void setStorageDir(RaftProperties properties, List<File> storageDir) {
setFiles(properties::setFiles, STORAGE_DIR_KEY, storageDir);
}
+ String STORAGE_SPACE_RESERVED_KEY = PREFIX + ".storage.space.reserved";
Review comment:
Hmmm..., but 'min' or 'required' does not match the meaning of this config?
The space configured here is reserved for final snapshot usage,
but 'min' or 'required' seems to configure some space for server to consume.
|---------------for server to consume-------------------| reserved |
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r643623331
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -51,6 +51,16 @@ static void setStorageDir(RaftProperties properties, List<File> storageDir) {
setFiles(properties::setFiles, STORAGE_DIR_KEY, storageDir);
}
+ String STORAGE_SPACE_RESERVED_KEY = PREFIX + ".storage.space.reserved";
Review comment:
Hmmm..., but 'min' or 'required' does not match the meaning of this config?
The space configured here is reserved for final snapshot usage,
but 'min' or 'required' seems to configure some space for server to consume.
|---------------for server to consume-------------------| ~reserved~ |
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on pull request #479:
URL: https://github.com/apache/ratis/pull/479#issuecomment-851362792
Hi @szetszwo @bshashikant , please help review this. The failed cases shall be unrelated as I run by hand locally.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r648190929
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -51,6 +51,16 @@ static void setStorageDir(RaftProperties properties, List<File> storageDir) {
setFiles(properties::setFiles, STORAGE_DIR_KEY, storageDir);
}
+ String STORAGE_SPACE_RESERVED_KEY = PREFIX + ".storage.space.reserved";
Review comment:
OK.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on pull request #479:
URL: https://github.com/apache/ratis/pull/479#issuecomment-854464656
ping @szetszwo
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on pull request #479: RATIS-1377. Ratis min free space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on pull request #479:
URL: https://github.com/apache/ratis/pull/479#issuecomment-858334368
@szetszwo updated, the failure with checkstyle should be related to the 'retrigger ci' commit, not related to the patch itself, thanks~
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng edited a comment on pull request #479: RATIS-1377. Ratis min free space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng edited a comment on pull request #479:
URL: https://github.com/apache/ratis/pull/479#issuecomment-858334368
@szetszwo updated, the failure with checkstyle should be related to the 'retrigger ci' commit, not related to the patch itself as I run successfully locally, thanks~
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #479: RATIS-1377. Ratis min free space for storage dirs.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #479:
URL: https://github.com/apache/ratis/pull/479#issuecomment-858347078
Checkstyle failed. Let's retry.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r643568848
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -51,6 +51,16 @@ static void setStorageDir(RaftProperties properties, List<File> storageDir) {
setFiles(properties::setFiles, STORAGE_DIR_KEY, storageDir);
}
+ String STORAGE_SPACE_RESERVED_KEY = PREFIX + ".storage.space.reserved";
Review comment:
Let's call it ".storage.space.min" or ".storage.space.required".
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageDirectoryImpl.java
##########
@@ -150,6 +152,13 @@ StorageState analyzeStorage(boolean toLock) throws IOException {
this.lock(); // lock storage if it exists
}
+ // check enough space
+ if (!hasEnoughSpace()) {
+ LOG.warn("There are not enough space left for directory " + rootPath
+ + "required: " + spaceReserved + " left: " + root.getFreeSpace());
+ return StorageState.NON_EXISTENT;
Review comment:
Let's add a new NO_SPACE state. Otherwise, StartupOption.FORMAT won't work.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageDirectoryImpl.java
##########
@@ -150,6 +152,13 @@ StorageState analyzeStorage(boolean toLock) throws IOException {
this.lock(); // lock storage if it exists
}
+ // check enough space
+ if (!hasEnoughSpace()) {
+ LOG.warn("There are not enough space left for directory " + rootPath
+ + "required: " + spaceReserved + " left: " + root.getFreeSpace());
Review comment:
Let's use "free space" and add some comma as below
`", required: " + spaceReserved + ", free space: " + root.getFreeSpace());`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r643621279
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageDirectoryImpl.java
##########
@@ -150,6 +152,13 @@ StorageState analyzeStorage(boolean toLock) throws IOException {
this.lock(); // lock storage if it exists
}
+ // check enough space
+ if (!hasEnoughSpace()) {
+ LOG.warn("There are not enough space left for directory " + rootPath
+ + "required: " + spaceReserved + " left: " + root.getFreeSpace());
+ return StorageState.NON_EXISTENT;
Review comment:
Oh, thanks for pointing this out, will fix it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r648166535
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -51,6 +51,16 @@ static void setStorageDir(RaftProperties properties, List<File> storageDir) {
setFiles(properties::setFiles, STORAGE_DIR_KEY, storageDir);
}
+ String STORAGE_SPACE_RESERVED_KEY = PREFIX + ".storage.space.reserved";
Review comment:
Sure, let's use '.storage.free-space.min'. Thanks.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guihecheng commented on a change in pull request #479: RATIS-1377. Ratis reserved space for storage dirs.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #479:
URL: https://github.com/apache/ratis/pull/479#discussion_r643623331
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -51,6 +51,16 @@ static void setStorageDir(RaftProperties properties, List<File> storageDir) {
setFiles(properties::setFiles, STORAGE_DIR_KEY, storageDir);
}
+ String STORAGE_SPACE_RESERVED_KEY = PREFIX + ".storage.space.reserved";
Review comment:
Hmmm..., but 'min' or 'required' does not match the meaning of this config? @szetszwo
The space configured here is reserved for final snapshot usage,
but 'min' or 'required' seems to configure some space for server to consume.
|---------------for server to consume-------------------| reserved |
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #479: RATIS-1377. Ratis min free space for storage dirs.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #479:
URL: https://github.com/apache/ratis/pull/479#issuecomment-858352757
It just has passed Checkstyle. Will merge this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo closed pull request #479: RATIS-1377. Ratis min free space for storage dirs.
Posted by GitBox <gi...@apache.org>.
szetszwo closed pull request #479:
URL: https://github.com/apache/ratis/pull/479
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org