kou commented on code in PR #38267:
URL: https://github.com/apache/arrow/pull/38267#discussion_r1360236958
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -2986,9 +2994,7 @@ Status InitializeS3(const S3GlobalOptions& options) {
return Status::OK();
}
-Status EnsureS3Initialized() {
- return EnsureAwsInstanceInitialized({S3LogLevel::Fatal}).status();
-}
+Status EnsureS3Initialized() { return EnsureAwsInstanceInitialized().status();
}
Review Comment:
How about passing `S3GlobalOptions::Defaults()` here instead of defining
`EnsureAwsInstanceInitialized(void)` because
`EnsureAwsInstanceInitialized(void)` will not be reused?
```suggestion
Status EnsureS3Initialized() { return
EnsureAwsInstanceInitialized(S3GlobalOptions::Defaults()).status(); }
```
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -3001,6 +3007,36 @@ bool IsS3Initialized() { return
GetAwsInstance()->IsInitialized(); }
bool IsS3Finalized() { return GetAwsInstance()->IsFinalized(); }
+S3GlobalOptions S3GlobalOptions::Defaults() {
+ S3LogLevel log_level = S3LogLevel::Fatal;
Review Comment:
Can we use `auto` here?
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -332,9 +332,24 @@ struct ARROW_EXPORT S3GlobalOptions {
///
/// For more details see Aws::Crt::Io::EventLoopGroup
int num_event_loop_threads = 1;
+
+ /// \brief Initialize with default options
+ ///
+ /// For log_level, this method first tries to extract a suitable value from
the
+ /// environment variable ARROW_S3_LOG_LEVEL.
+ static S3GlobalOptions Defaults();
};
-/// \brief Initialize the S3 APIs.
+/// \brief Initialize the S3 APIs with default options.
+///
+/// It is required to call this function at least once before using
S3FileSystem.
+///
+/// Once this function is called you MUST call FinalizeS3 before the end of the
+/// application in order to avoid a segmentation fault at shutdown.
+ARROW_EXPORT
+Status InitializeS3();
Review Comment:
I think that we don't need to provided this because `EnsureS3Initialized()`
will be used when a user wants to use the default S3 options.
Do you have any use-case for this?
##########
cpp/src/arrow/filesystem/s3fs_test.cc:
##########
@@ -1380,5 +1380,33 @@ class TestS3FSGeneric : public S3TestMixin, public
GenericFileSystemTest {
GENERIC_FS_TEST_FUNCTIONS(TestS3FSGeneric);
+////////////////////////////////////////////////////////////////////////////
+// S3GlobalOptionsDefaults tests
Review Comment:
```suggestion
// S3GlobalOptions::Defaults tests
```
Or `S3GlobalOptions tests`.
##########
cpp/src/arrow/filesystem/s3fs_test.cc:
##########
@@ -1380,5 +1380,33 @@ class TestS3FSGeneric : public S3TestMixin, public
GenericFileSystemTest {
GENERIC_FS_TEST_FUNCTIONS(TestS3FSGeneric);
+////////////////////////////////////////////////////////////////////////////
+// S3GlobalOptionsDefaults tests
+
+class S3GlobalOptionsDefaults : public ::testing::Test {};
+
+TEST_F(S3GlobalOptionsDefaults, S3GlobalOptionsDefaultsLogLevel) {
+ // Verify we get the default value of Fatal
+ ASSERT_EQ(S3LogLevel::Fatal,
arrow::fs::S3GlobalOptions::Defaults().log_level);
+
+ // Verify we get the value specified by env var and not the default
+ {
+ EnvVarGuard log_level_guard("ARROW_S3_LOG_LEVEL", "Error");
Review Comment:
How about using upper case style here because we use upper case in our
document?
```suggestion
EnvVarGuard log_level_guard("ARROW_S3_LOG_LEVEL", "ERROR");
```
##########
cpp/src/arrow/filesystem/s3fs_test.cc:
##########
@@ -1380,5 +1380,33 @@ class TestS3FSGeneric : public S3TestMixin, public
GenericFileSystemTest {
GENERIC_FS_TEST_FUNCTIONS(TestS3FSGeneric);
+////////////////////////////////////////////////////////////////////////////
+// S3GlobalOptionsDefaults tests
+
+class S3GlobalOptionsDefaults : public ::testing::Test {};
+
+TEST_F(S3GlobalOptionsDefaults, S3GlobalOptionsDefaultsLogLevel) {
Review Comment:
It seems that we don't need to use fixture and simplify test name:
```suggestion
TEST(S3GlobalOptions, DefaultsLogLevel) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
