You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/20 19:36:31 UTC
[GitHub] [arrow] wjones127 opened a new pull request, #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete buckets
wjones127 opened a new pull request, #13206:
URL: https://github.com/apache/arrow/pull/13206
BREAKING CHANGE: modifies `S3FileSystem` to not allow creating or deleting buckets by default. A new option is added `allow_create_buckets`, which enables creating and deleting buckets.
Outside of tests, most use cases will not want to create or delete buckets, and doing so accidentally is not desirable either. Buckets have governance controls like permissions and cost-tracking tags.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r888222522
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -218,6 +229,12 @@ class ARROW_EXPORT S3FileSystem : public FileSystem {
/// Return the actual region this filesystem connects to
std::string region() const;
+ /// Set create_buckets property of options
Review Comment:
Okay, I think I'm fine at this point with only allowing setting them during construction, since we've added them to the URI parsing.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on PR #13206:
URL: https://github.com/apache/arrow/pull/13206#issuecomment-1136497947
> Could you send an e-mail to dev@ to confirm whether this default behavior change is acceptable? I can't decide this because I haven't used Apache Arrow to access S3 yet.
Sure, I've done that here: https://lists.apache.org/thread/2wnmq72trrtcxyvxzw261gq0t6grq18g
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r890299274
##########
r/tests/testthat/test-s3-minio.R:
##########
@@ -180,6 +190,20 @@ if (arrow_with_s3() && process_is_running("minio server")) {
expect_length(fs$ls(minio_path("new_dataset_dir")), 1)
})
+ test_that("CreateDir fails on bucket if allow_bucket_creation=False", {
+ now_tmp <- paste0(now, "-test-fail-delete")
+ fs$CreateDir(now_tmp)
+
+ expect_error(
+ limited_fs$CreateDir("should-fail"),
+ regexp = "Bucket does not exist"
+ )
+ expect_error(
+ limited_fs$DeleteDir(now_tmp),
+ regexp = "Would delete bucket"
+ )
+ })
Review Comment:
I wonder if these tests might be better without the `regexp` parameter, given that the specific phrasing of them comes from the C++ layer, and we tend to avoid testing that component of the error message?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r879687343
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -130,6 +130,9 @@ struct ARROW_EXPORT S3Options {
/// Whether OutputStream writes will be issued in the background, without blocking.
bool background_writes = true;
+ /// Whether to allow creation of new buckets
+ bool allow_create_buckets = false;
Review Comment:
Good idea. I've added 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r886614745
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -218,6 +229,12 @@ class ARROW_EXPORT S3FileSystem : public FileSystem {
/// Return the actual region this filesystem connects to
std::string region() const;
+ /// Set create_buckets property of options
Review Comment:
These trivial setters are not needed as people can access the attributes directly.
##########
r/R/filesystem.R:
##########
@@ -180,6 +184,11 @@ FileSelector$create <- function(base_dir, allow_not_found = FALSE, recursive = F
#' - `$OpenAppendStream(path)`: Open an [output stream][OutputStream] for
#' appending.
#'
+#' `S3FileSystem` also has methods:
+#'
+#' - `$allow_bucket_creation()` which can set the corresponding option above.
+#' - `$allow_bucket_deletion()` which can set the corresponding option above.
Review Comment:
Why expose these as methods as well? Is it useful?
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -353,6 +354,12 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
options.scheme = kv.second;
} else if (kv.first == "endpoint_override") {
options.endpoint_override = kv.second;
+ } else if (kv.first == "allow_bucket_creation") {
+ options.allow_bucket_creation =
+ ::arrow::internal::AsciiEqualsCaseInsensitive(kv.second, "true");
+ } else if (kv.first == "allow_bucket_deletion") {
+ options.allow_bucket_deletion =
+ ::arrow::internal::AsciiEqualsCaseInsensitive(kv.second, "true");
Review Comment:
Hmm, I think we should be a bit stricter and not blindly interpret any other value as "false".
How about allowing the following values:
* "0", "false" (case-insensitive) -> boolean false
* "1", "true" (case-insensitive) -> boolean true
* any other value -> raise `Status::Invalid`
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -130,6 +130,17 @@ struct ARROW_EXPORT S3Options {
/// Whether OutputStream writes will be issued in the background, without blocking.
bool background_writes = true;
+ /// Whether to allow creation or deletion of buckets
Review Comment:
```suggestion
/// Whether to allow creation of buckets
```
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -312,3 +328,25 @@ cdef class S3FileSystem(FileSystem):
The AWS region this filesystem connects to.
"""
return frombytes(self.s3fs.region())
+
+ @property
+ def allow_bucket_creation(self):
+ """
+ Whether to allow CreateDir at the bucket-level.
Review Comment:
We should probably use the same docstring for constructor parameters and the associated properties.
Personally, I like "Whether to allow CreateDir at the bucket-level."
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -193,6 +195,8 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
CResult[shared_ptr[CS3FileSystem]] Make(const CS3Options& options)
CS3Options options()
c_string region()
+ void allow_bucket_creation(c_bool allow)
+ void allow_bucket_deletion(c_bool allow)
Review Comment:
Not needed?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13206:
URL: https://github.com/apache/arrow/pull/13206#issuecomment-1147391614
@thisisnic @dragosmg Can one of you perhaps review the R changes?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r886185955
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -130,6 +130,9 @@ struct ARROW_EXPORT S3Options {
/// Whether OutputStream writes will be issued in the background, without blocking.
bool background_writes = true;
+ /// Whether to allow creation of new buckets
+ bool allow_create_buckets = false;
Review Comment:
I liked `create_buckets` but since there is a setter method and I want it named the same thing as the property (for clear error hints), I felt like `allow_bucket_creation` was the better choice.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r881906038
##########
python/pyarrow/tests/test_dataset.py:
##########
@@ -4505,9 +4507,21 @@ def test_write_dataset_s3_put_only(s3_server):
).to_table()
assert result.equals(table)
- with pytest.raises(OSError, match="Access Denied"):
+ # Passing create_dir is fine if the bucket already exists
+ ds.write_dataset(
+ table, "existing-bucket", filesystem=fs,
+ format="feather", create_dir=True, partitioning=part,
+ existing_data_behavior='overwrite_or_ignore'
+ )
+ # check roundtrip
+ result = ds.dataset(
+ "existing-bucket", filesystem=fs, format="ipc", partitioning="hive"
+ ).to_table()
+ assert result.equals(table)
+
+ with pytest.raises(OSError, match="Bucket does not exist"):
ds.write_dataset(
- table, "existing-bucket", filesystem=fs,
+ table, "non-existing-bucket", filesystem=fs,
Review Comment:
Sure, though I think I want to test both 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r881325065
##########
python/pyarrow/tests/test_dataset.py:
##########
@@ -4505,9 +4507,21 @@ def test_write_dataset_s3_put_only(s3_server):
).to_table()
assert result.equals(table)
- with pytest.raises(OSError, match="Access Denied"):
+ # Passing create_dir is fine if the bucket already exists
+ ds.write_dataset(
+ table, "existing-bucket", filesystem=fs,
+ format="feather", create_dir=True, partitioning=part,
+ existing_data_behavior='overwrite_or_ignore'
+ )
+ # check roundtrip
+ result = ds.dataset(
+ "existing-bucket", filesystem=fs, format="ipc", partitioning="hive"
+ ).to_table()
+ assert result.equals(table)
+
+ with pytest.raises(OSError, match="Bucket does not exist"):
ds.write_dataset(
- table, "existing-bucket", filesystem=fs,
+ table, "non-existing-bucket", filesystem=fs,
Review Comment:
To retain the original purpose of the test, should we use an `fs` with allow_create_bucket set to True? So that it raises the original error? (even if the user sets this parameter to True, we still need to honor the limitations / the create_dir keyword)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete buckets
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13206:
URL: https://github.com/apache/arrow/pull/13206#issuecomment-1133258853
https://issues.apache.org/jira/browse/ARROW-15906
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r885543144
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -130,6 +130,9 @@ struct ARROW_EXPORT S3Options {
/// Whether OutputStream writes will be issued in the background, without blocking.
bool background_writes = true;
+ /// Whether to allow creation of new buckets
+ bool allow_create_buckets = false;
Review Comment:
Perhaps `create_buckets` and `delete_buckets`? That reads more consistently with `background_writes` to me. Or `allow_creating_buckets`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r888083810
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -218,6 +229,12 @@ class ARROW_EXPORT S3FileSystem : public FileSystem {
/// Return the actual region this filesystem connects to
std::string region() const;
+ /// Set create_buckets property of options
Review Comment:
> These trivial setters are not needed as people can access the attributes directly.
I'm not so sure that's true. This fails:
```cpp
auto options = fs_->options();
options.allow_bucket_creation = true;
options.allow_bucket_deletion = true;
ASSERT_EQ(fs_->options().allow_bucket_creation, true);
ASSERT_EQ(fs_->options().allow_bucket_deletion, 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r884945051
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -130,6 +130,9 @@ struct ARROW_EXPORT S3Options {
/// Whether OutputStream writes will be issued in the background, without blocking.
bool background_writes = true;
+ /// Whether to allow creation of new buckets
+ bool allow_create_buckets = false;
Review Comment:
Let's make it two options for flexibility (creation and deletion).
Also wrt. naming, should this be `allow_bucket_creation`? @lidavidm
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou merged pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
pitrou merged PR #13206:
URL: https://github.com/apache/arrow/pull/13206
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r881037233
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -130,6 +130,14 @@ struct ARROW_EXPORT S3Options {
/// Whether OutputStream writes will be issued in the background, without blocking.
bool background_writes = true;
+ /// Whether to allow creation or deletion of buckets
Review Comment:
We probably don't have to. Buckets can only be deleted if already empty, so hard to accidentally delete an important one I suppose.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r888084870
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -218,6 +229,12 @@ class ARROW_EXPORT S3FileSystem : public FileSystem {
/// Return the actual region this filesystem connects to
std::string region() const;
+ /// Set create_buckets property of options
Review Comment:
Hmm, this is not really supported, though. You should set options before creating the filesystem.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r886897936
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -353,6 +354,12 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
options.scheme = kv.second;
} else if (kv.first == "endpoint_override") {
options.endpoint_override = kv.second;
+ } else if (kv.first == "allow_bucket_creation") {
+ options.allow_bucket_creation =
+ ::arrow::internal::AsciiEqualsCaseInsensitive(kv.second, "true");
+ } else if (kv.first == "allow_bucket_deletion") {
+ options.allow_bucket_deletion =
+ ::arrow::internal::AsciiEqualsCaseInsensitive(kv.second, "true");
Review Comment:
Sounds good. I didn't see any existing utility function to do this, so maybe I will create one.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r890575333
##########
r/tests/testthat/test-s3-minio.R:
##########
@@ -180,6 +190,20 @@ if (arrow_with_s3() && process_is_running("minio server")) {
expect_length(fs$ls(minio_path("new_dataset_dir")), 1)
})
+ test_that("CreateDir fails on bucket if allow_bucket_creation=False", {
+ now_tmp <- paste0(now, "-test-fail-delete")
+ fs$CreateDir(now_tmp)
+
+ expect_error(
+ limited_fs$CreateDir("should-fail"),
+ regexp = "Bucket does not exist"
+ )
+ expect_error(
+ limited_fs$DeleteDir(now_tmp),
+ regexp = "Would delete bucket"
+ )
+ })
Review Comment:
Sure, I'll rewrite it so these messages are tested in C++ 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r880951359
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -130,6 +130,14 @@ struct ARROW_EXPORT S3Options {
/// Whether OutputStream writes will be issued in the background, without blocking.
bool background_writes = true;
+ /// Whether to allow creation or deletion of buckets
Review Comment:
It seems that "deletion" depends on `allow_create_buckets` confuses users. How about renaming `allow_create_buckets` or create one more option?
BTW, why should we disable "deletion" by default?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r878756026
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -130,6 +130,9 @@ struct ARROW_EXPORT S3Options {
/// Whether OutputStream writes will be issued in the background, without blocking.
bool background_writes = true;
+ /// Whether to allow creation of new buckets
+ bool allow_create_buckets = false;
Review Comment:
Could you also update `S3Options::FromUri()` to support 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r886899734
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -218,6 +229,12 @@ class ARROW_EXPORT S3FileSystem : public FileSystem {
/// Return the actual region this filesystem connects to
std::string region() const;
+ /// Set create_buckets property of options
Review Comment:
`options()` returns a const reference; they could access but I don't think they could mutate right?
Being able to change this setting later felt important when I didn't support in `FromURI`, but maybe it's not as important now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13206:
URL: https://github.com/apache/arrow/pull/13206#discussion_r890095199
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -253,6 +266,9 @@ cdef class S3FileSystem(FileSystem):
"'proxy_options': expected 'dict' or 'str', "
f"got {type(proxy_options)} instead.")
+ options.allow_bucket_creation = allow_bucket_creation
+ options.allow_bucket_deletion = allow_bucket_deletion
+
Review Comment:
You'll also need to update `__reduce__` below.
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -212,7 +212,9 @@ def s3fs(request, s3_server):
access_key=access_key,
secret_key=secret_key,
endpoint_override='{}:{}'.format(host, port),
- scheme='http'
+ scheme='http',
+ allow_bucket_creation=True,
+ allow_bucket_deletion=True
)
fs.create_dir(bucket)
Review Comment:
Can you also add tests in `test_s3_options`?
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -353,6 +354,12 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
options.scheme = kv.second;
} else if (kv.first == "endpoint_override") {
options.endpoint_override = kv.second;
+ } else if (kv.first == "allow_bucket_creation") {
+ ARROW_ASSIGN_OR_RAISE(options.allow_bucket_creation,
+ ::arrow::internal::ParseBoolean(kv.second));
+ } else if (kv.first == "allow_bucket_deletion") {
+ ARROW_ASSIGN_OR_RAISE(options.allow_bucket_deletion,
+ ::arrow::internal::ParseBoolean(kv.second));
Review Comment:
Can you also update `S3Options::Equals`?
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -443,6 +445,12 @@ def test_s3fs_limited_permissions_create_bucket(s3_server):
)
fs.create_dir('existing-bucket/test')
+ with pytest.raises(pa.ArrowIOError):
Review Comment:
Perhaps also add a `match=` argument to also check relevant part of the error message?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] dragosmg commented on pull request #13206: ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets
Posted by GitBox <gi...@apache.org>.
dragosmg commented on PR #13206:
URL: https://github.com/apache/arrow/pull/13206#issuecomment-1147523279
LGTM, but I'd like the opinion of someone more experienced.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org