You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "suyanhanx (via GitHub)" <gi...@apache.org> on 2023/09/08 09:42:36 UTC
[GitHub] [incubator-opendal] suyanhanx opened a new pull request, #3023: test(blocking): tests for blocking append
suyanhanx opened a new pull request, #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023
Although there are not many services having native implementations of blocking_write now, we still need to cover them.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo merged pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1319660670
##########
core/src/services/fs/backend.rs:
##########
@@ -566,10 +566,15 @@ impl Accessor for FsBackend {
(p, None)
};
- let f = std::fs::OpenOptions::new()
- .create(true)
- .truncate(true)
- .write(true)
+ let mut open_option = std::fs::OpenOptions::new();
+ open_option.create(true).write(true);
+ if args.append() {
Review Comment:
https://github.com/apache/incubator-opendal/pull/3023#issuecomment-1711400298
That's exactly what I'm talking about.
And this is not the bug of this part you referenced.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1321600077
##########
core/src/services/fs/writer.rs:
##########
@@ -58,9 +62,23 @@ impl oio::Write for FsWriter<tokio::fs::File> {
self.f.sync_all().await.map_err(parse_io_error)?;
if let Some(tmp_path) = &self.tmp_path {
- tokio::fs::rename(tmp_path, &self.target_path)
- .await
- .map_err(parse_io_error)?;
+ if self.op.append() {
Review Comment:
> I believe it is not possible to perform this action. If users attempt to add 1MiB of data to a 10GiB file, they will be surprised by the outcome.
I understand what you're saying. This will result in 2x the amount of data being written.🤔
But for atomic, it will be inevitable if the file exists. 😢
##########
core/src/services/fs/writer.rs:
##########
@@ -58,9 +62,23 @@ impl oio::Write for FsWriter<tokio::fs::File> {
self.f.sync_all().await.map_err(parse_io_error)?;
if let Some(tmp_path) = &self.tmp_path {
- tokio::fs::rename(tmp_path, &self.target_path)
- .await
- .map_err(parse_io_error)?;
+ if self.op.append() {
Review Comment:
> I believe it is not possible to perform this action. If users attempt to add 1MiB of data to a 10GiB file, they will be surprised by the outcome.
I understand what you're saying. This will result in 2x the amount of data being written.🤔
But for atomic, it will be inevitable if the file exists. 😢
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on pull request #3023: test(blocking): tests for blocking append
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#issuecomment-1711400298
https://github.com/apache/incubator-opendal/blob/1bb63ab676293cb20cb90fbb177d58bd25340c8c/core/src/services/fs/writer.rs#L57-L68
For the service fs, doing append will go wrong definitely here if we enable atomic_write.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1320786085
##########
core/src/services/fs/writer.rs:
##########
@@ -58,9 +62,23 @@ impl oio::Write for FsWriter<tokio::fs::File> {
self.f.sync_all().await.map_err(parse_io_error)?;
if let Some(tmp_path) = &self.tmp_path {
- tokio::fs::rename(tmp_path, &self.target_path)
- .await
- .map_err(parse_io_error)?;
+ if self.op.append() {
Review Comment:
I believe it is not possible to perform this action. If users attempt to add 1MiB of data to a 10GiB file, they will be surprised by the outcome.
If the append function is enabled, we need to check if the file exists. If the file does not exist, we should create a new file, append and then rename it (like what we do for `write`). Otherwise, we can directly append to the existing file.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1328246189
##########
core/src/services/fs/backend.rs:
##########
@@ -367,15 +372,23 @@ impl Accessor for FsBackend {
let target_path = Self::ensure_write_abs_path(&self.root, path).await?;
let tmp_path =
Self::ensure_write_abs_path(atomic_write_dir, &tmp_file_of(path)).await?;
- (target_path, Some(tmp_path))
+
+ // If the target file exists, we should append to the end of it directly.
+ if tokio::fs::try_exists(&target_path)
Review Comment:
This should happen if args.append is true.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1319653108
##########
core/src/services/fs/backend.rs:
##########
@@ -566,10 +566,15 @@ impl Accessor for FsBackend {
(p, None)
};
- let f = std::fs::OpenOptions::new()
- .create(true)
- .truncate(true)
- .write(true)
+ let mut open_option = std::fs::OpenOptions::new();
+ open_option.create(true).write(true);
+ if args.append() {
Review Comment:
We should handle append carefully since the file could already been exist.
##########
core/tests/behavior/main.rs:
##########
@@ -41,13 +41,15 @@ use read_only::behavior_read_only_tests;
use rename::behavior_rename_tests;
use write::behavior_write_tests;
+mod blocking_append;
Review Comment:
Maybe put under `// Blocking test cases`?
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1328242980
##########
core/src/services/fs/backend.rs:
##########
@@ -566,10 +578,24 @@ impl Accessor for FsBackend {
(p, None)
};
- let f = std::fs::OpenOptions::new()
- .create(true)
- .truncate(true)
- .write(true)
+ let mut f = std::fs::OpenOptions::new();
+ f.create(true).write(true);
+
+ if op.append() {
+ f.append(true);
+
+ // If the target file exists, we should append to the end of it directly.
+ if Path::new(&target_path)
Review Comment:
How about do this check in `if let Some(atomic_write_dir) {}` block, so that we can avoid those cost if `atomic_write` not enabled.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1319659140
##########
core/src/services/fs/backend.rs:
##########
@@ -566,10 +566,15 @@ impl Accessor for FsBackend {
(p, None)
};
- let f = std::fs::OpenOptions::new()
- .create(true)
- .truncate(true)
- .write(true)
+ let mut open_option = std::fs::OpenOptions::new();
+ open_option.create(true).write(true);
+ if args.append() {
Review Comment:
Think about this case: If `atmoic_write` has been enabled, user trying to `append` a file. Current logic will wirte a new file in tmp_path and rename to target_path which will overwrite the existing file.
##########
core/src/services/fs/backend.rs:
##########
@@ -566,10 +566,15 @@ impl Accessor for FsBackend {
(p, None)
};
- let f = std::fs::OpenOptions::new()
- .create(true)
- .truncate(true)
- .write(true)
+ let mut open_option = std::fs::OpenOptions::new();
+ open_option.create(true).write(true);
+ if args.append() {
Review Comment:
Think about this case: If `atmoic_write` has been enabled, user trying to `append` a file. Current logic will write a new file in tmp_path and rename to target_path which will overwrite the existing file.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1328246469
##########
core/src/services/fs/backend.rs:
##########
@@ -367,15 +372,23 @@ impl Accessor for FsBackend {
let target_path = Self::ensure_write_abs_path(&self.root, path).await?;
let tmp_path =
Self::ensure_write_abs_path(atomic_write_dir, &tmp_file_of(path)).await?;
- (target_path, Some(tmp_path))
+
+ // If the target file exists, we should append to the end of it directly.
+ if tokio::fs::try_exists(&target_path)
Review Comment:
🥲
##########
core/src/services/fs/backend.rs:
##########
@@ -367,15 +372,23 @@ impl Accessor for FsBackend {
let target_path = Self::ensure_write_abs_path(&self.root, path).await?;
let tmp_path =
Self::ensure_write_abs_path(atomic_write_dir, &tmp_file_of(path)).await?;
- (target_path, Some(tmp_path))
+
+ // If the target file exists, we should append to the end of it directly.
+ if tokio::fs::try_exists(&target_path)
Review Comment:
🥲
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1328251832
##########
website/blog/2023-09-14-owo-1/index.md:
##########
@@ -43,6 +43,8 @@ w.close().await?;
This way, users can reuse the `Writer` in their own logic without handling `append` separately.
+The atomic write will not make effects if the writer is in append mode.
Review Comment:
Please don't update my posts, 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.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1321604101
##########
core/src/services/fs/writer.rs:
##########
@@ -58,9 +62,23 @@ impl oio::Write for FsWriter<tokio::fs::File> {
self.f.sync_all().await.map_err(parse_io_error)?;
if let Some(tmp_path) = &self.tmp_path {
- tokio::fs::rename(tmp_path, &self.target_path)
- .await
- .map_err(parse_io_error)?;
+ if self.op.append() {
Review Comment:
> But for atomic, it will be inevitable if the file exists. 😢
We can append to existing file if `append` is true, it's the best thing we can do.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1319635861
##########
core/tests/behavior/write.rs:
##########
@@ -47,6 +47,7 @@ pub fn behavior_write_tests(op: &Operator) -> Vec<Trial> {
test_create_dir,
test_create_dir_existing,
test_write_only,
+ test_write_empty_file,
Review Comment:
No all services support write empty file, we should add a new capability for this. Maybe it's better to split into a new PR instead?
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1319637495
##########
core/tests/behavior/write.rs:
##########
@@ -47,6 +47,7 @@ pub fn behavior_write_tests(op: &Operator) -> Vec<Trial> {
test_create_dir,
test_create_dir_existing,
test_write_only,
+ test_write_empty_file,
Review Comment:
I've removed it. Will do this in a new PR.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1319656607
##########
core/src/services/fs/backend.rs:
##########
@@ -566,10 +566,15 @@ impl Accessor for FsBackend {
(p, None)
};
- let f = std::fs::OpenOptions::new()
- .create(true)
- .truncate(true)
- .write(true)
+ let mut open_option = std::fs::OpenOptions::new();
+ open_option.create(true).write(true);
+ if args.append() {
Review Comment:
What do you mean? 🤔 Is there any problem?
##########
core/src/services/fs/backend.rs:
##########
@@ -566,10 +566,15 @@ impl Accessor for FsBackend {
(p, None)
};
- let f = std::fs::OpenOptions::new()
- .create(true)
- .truncate(true)
- .write(true)
+ let mut open_option = std::fs::OpenOptions::new();
+ open_option.create(true).write(true);
+ if args.append() {
Review Comment:
What do you mean? 🤔 Is there any problem?
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#issuecomment-1711408524
> It's better to set a new test job for atomic_write.
Negative, this is a violation of our VISION. We can't add different test for services with different configure values.
> For the service fs, doing append will go wrong definitely here if we enable atomic_write.
It's a bug. We should fix it from fs internal.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1319660670
##########
core/src/services/fs/backend.rs:
##########
@@ -566,10 +566,15 @@ impl Accessor for FsBackend {
(p, None)
};
- let f = std::fs::OpenOptions::new()
- .create(true)
- .truncate(true)
- .write(true)
+ let mut open_option = std::fs::OpenOptions::new();
+ open_option.create(true).write(true);
+ if args.append() {
Review Comment:
https://github.com/apache/incubator-opendal/pull/3023#issuecomment-1711400298
That's exactly what I'm talking about.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1321604101
##########
core/src/services/fs/writer.rs:
##########
@@ -58,9 +62,23 @@ impl oio::Write for FsWriter<tokio::fs::File> {
self.f.sync_all().await.map_err(parse_io_error)?;
if let Some(tmp_path) = &self.tmp_path {
- tokio::fs::rename(tmp_path, &self.target_path)
- .await
- .map_err(parse_io_error)?;
+ if self.op.append() {
Review Comment:
> But for atomic, it will be inevitable if the file exists. 😢
We can append to existing file if `append` is true.
##########
core/src/services/fs/writer.rs:
##########
@@ -58,9 +62,23 @@ impl oio::Write for FsWriter<tokio::fs::File> {
self.f.sync_all().await.map_err(parse_io_error)?;
if let Some(tmp_path) = &self.tmp_path {
- tokio::fs::rename(tmp_path, &self.target_path)
- .await
- .map_err(parse_io_error)?;
+ if self.op.append() {
Review Comment:
> But for atomic, it will be inevitable if the file exists. 😢
We can append to existing file if `append` is true.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #3023: test(blocking): tests for blocking append
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3023:
URL: https://github.com/apache/incubator-opendal/pull/3023#discussion_r1328243487
##########
core/src/services/fs/backend.rs:
##########
@@ -566,10 +578,24 @@ impl Accessor for FsBackend {
(p, None)
};
- let f = std::fs::OpenOptions::new()
- .create(true)
- .truncate(true)
- .write(true)
+ let mut f = std::fs::OpenOptions::new();
+ f.create(true).write(true);
+
+ if op.append() {
+ f.append(true);
+
+ // If the target file exists, we should append to the end of it directly.
+ if Path::new(&target_path)
Review Comment:
Make sense.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org