You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "szetszwo (via GitHub)" <gi...@apache.org> on 2023/10/22 17:42:25 UTC
[PR] RATIS-1915. Do not use FileInputStream/FileOutputStream in ratis-common. [ratis]
szetszwo opened a new pull request, #947:
URL: https://github.com/apache/ratis/pull/947
See RATIS-1915
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-1915. Do not use FileInputStream/FileOutputStream in ratis-common. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #947:
URL: https://github.com/apache/ratis/pull/947#issuecomment-1777665533
@adoroszlai , it may be related to the change of 4KB data to 4MB in the test since the test setSnapshotChunkSizeMax to 1KB. Let's revert 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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-1915. Do not use FileInputStream/FileOutputStream in ratis-common. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #947:
URL: https://github.com/apache/ratis/pull/947
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-1915. Do not use FileInputStream/FileOutputStream in ratis-common. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #947:
URL: https://github.com/apache/ratis/pull/947#issuecomment-1780028983
@adoroszlai , thanks a lot for reviewing 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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-1915. Do not use FileInputStream/FileOutputStream in ratis-common. [ratis]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #947:
URL: https://github.com/apache/ratis/pull/947#discussion_r1369714170
##########
ratis-server/src/test/java/org/apache/ratis/InstallSnapshotFromLeaderTests.java:
##########
@@ -168,12 +169,12 @@ public long takeSnapshot() {
FileUtils.createDirectories(snapshotRoot);
FileUtils.createDirectories(file1.getParentFile());
FileUtils.createDirectories(file2.getParentFile());
- FileUtils.createNewFile(file1.toPath());
- FileUtils.createNewFile(file2.toPath());
+ FileUtils.newOutputStream(file1.toPath(), StandardOpenOption.CREATE_NEW);
+ FileUtils.newOutputStream(file2.toPath(), StandardOpenOption.CREATE_NEW);
Review Comment:
My point is that the test is leaking these two output streams. It's not a big deal, since it's just a unit test.
> these files should be closed locally
Can you please clarify that?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-1915. Do not use FileInputStream/FileOutputStream in ratis-common. [ratis]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #947:
URL: https://github.com/apache/ratis/pull/947#issuecomment-1777599206
Thanks @szetszwo for updating the patch, LGTM. There were some failures in `TestLeaderInstallSnapshot`, but I see it passed in your fork. Has this test been flaky before?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-1915. Do not use FileInputStream/FileOutputStream in ratis-common. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #947:
URL: https://github.com/apache/ratis/pull/947#discussion_r1369695976
##########
ratis-server/src/test/java/org/apache/ratis/InstallSnapshotFromLeaderTests.java:
##########
@@ -168,12 +169,12 @@ public long takeSnapshot() {
FileUtils.createDirectories(snapshotRoot);
FileUtils.createDirectories(file1.getParentFile());
FileUtils.createDirectories(file2.getParentFile());
- FileUtils.createNewFile(file1.toPath());
- FileUtils.createNewFile(file2.toPath());
+ FileUtils.newOutputStream(file1.toPath(), StandardOpenOption.CREATE_NEW);
+ FileUtils.newOutputStream(file2.toPath(), StandardOpenOption.CREATE_NEW);
Review Comment:
@adoroszlai , the test tries to create a few different snapshot files and test if they can be transferred correctly. So these files should be closed 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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-1915. Do not use FileInputStream/FileOutputStream in ratis-common. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #947:
URL: https://github.com/apache/ratis/pull/947#discussion_r1369893970
##########
ratis-server/src/test/java/org/apache/ratis/InstallSnapshotFromLeaderTests.java:
##########
@@ -168,12 +169,12 @@ public long takeSnapshot() {
FileUtils.createDirectories(snapshotRoot);
FileUtils.createDirectories(file1.getParentFile());
FileUtils.createDirectories(file2.getParentFile());
- FileUtils.createNewFile(file1.toPath());
- FileUtils.createNewFile(file2.toPath());
+ FileUtils.newOutputStream(file1.toPath(), StandardOpenOption.CREATE_NEW);
+ FileUtils.newOutputStream(file2.toPath(), StandardOpenOption.CREATE_NEW);
Review Comment:
Oops, sorry, I have not pushed the last commit.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-1915. Do not use FileInputStream/FileOutputStream in ratis-common. [ratis]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #947:
URL: https://github.com/apache/ratis/pull/947#discussion_r1369636769
##########
ratis-server/src/test/java/org/apache/ratis/InstallSnapshotFromLeaderTests.java:
##########
@@ -168,12 +169,12 @@ public long takeSnapshot() {
FileUtils.createDirectories(snapshotRoot);
FileUtils.createDirectories(file1.getParentFile());
FileUtils.createDirectories(file2.getParentFile());
- FileUtils.createNewFile(file1.toPath());
- FileUtils.createNewFile(file2.toPath());
+ FileUtils.newOutputStream(file1.toPath(), StandardOpenOption.CREATE_NEW);
+ FileUtils.newOutputStream(file2.toPath(), StandardOpenOption.CREATE_NEW);
Review Comment:
Shouldn't these be closed immediately? (applies to old code, too)
What is the benefit compared to the following?
```
file1.createNewFile();
file2.createNewFile();
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org