amoeba commented on PR #35398: URL: https://github.com/apache/arrow/pull/35398#issuecomment-1533867236
Thanks for taking a look @thisisnic. > I'm a little bit hesitant about using a function that sets state in a session in this way; there's something which feels a little unidiomatic. Yeah I agree, esp about it not feeling idiomatic, though I think it's hard to avoid due to how the AWS C++ SDK is designed. Reading https://docs.aws.amazon.com/sdk-for-cpp/v1/developer-guide/basic-use.html, it looks like any session should init the API once, which is the point where the application specifies the log level it desires for the remainder of the session. I think we could totally do something with options as you suggest but it would make it easier for the user to run into this scenario: ```r options(arrow.s3_log_level = "fatal") s3_bucket() # Indirectly causes initialization of the API with log level set to Fatal options(arrow.s3_log_level = "debug") # <---- Has no effect, log level stays set to Fatal ``` With the implementation in this PR, the user would instead get an error: ```r > arrow:::s3_init(log_level = "debug") Error: Invalid: S3 was already initialized. It is safe to use but the options passed in this call have been ignored. ``` > Actually, was this always intended to be an internal function (as I see it's not been exported), or was the @export tag missed? I left it unexported just until it was discussed in review and since I wasn't sure. I'm leaning towards export but am curious what you think. -- 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]
