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 2020/09/24 16:39:51 UTC

[GitHub] [arrow] pitrou opened a new pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

pitrou opened a new pull request #8261:
URL: https://github.com/apache/arrow/pull/8261


   Avoid trying to following redirects when resolving a region, as that can fail
   (at least on Windows with certain AWS SDK versions).
   
   Also skip a test that's failing because of a bug in the AWS SDK on Windows.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] bkietz commented on a change in pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#discussion_r494532469



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       ```diff
   -void DisableRedirectsImpl(bool* followRedirects) { *followRedirects = false; }
   +template <bool Never = false>
   +void DisableRedirectsImpl(bool* followRedirects) { *followRedirects = Never; }
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#discussion_r494496745



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       `if constexpr` would have been nice...




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] nealrichardson commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698499743


   @github-actions crossbow submit homebrew-r-autobrew


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] bkietz closed pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #8261:
URL: https://github.com/apache/arrow/pull/8261


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] nealrichardson commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698499743


   @github-actions crossbow submit homebrew-r-autobrew


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698479206






----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] bkietz closed pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #8261:
URL: https://github.com/apache/arrow/pull/8261


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] bkietz commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698588161


   Passing Travis: https://travis-ci.org/github/pitrou/arrow/builds/730048965


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] bkietz commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698588161


   Passing Travis: https://travis-ci.org/github/pitrou/arrow/builds/730048965


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] bkietz commented on a change in pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#discussion_r494491830



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       Maybe a little better?
   ```suggestion
   // In AWS SDK < 1.8, Aws::Client::ClientConfiguration::followRedirects is a bool.
   void DisableRedirectsImpl(bool* followRedirects) { *followRedirects = false; }
   
   // In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
   template <typename PolicyEnum, PolicyEnum Never = PolicyEnum::NEVER>
   void DisableRedirectsImpl(PolicyEnum* followRedirects) { *followRedirects = Never; }
   
   void DisableRedirects(Config* c) {
     DisableRedirectsImpl(&c->followRedirects);
   }
   ```

##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       ```diff
   -void DisableRedirectsImpl(bool* followRedirects) { *followRedirects = false; }
   +template <bool Never = false>
   +void DisableRedirectsImpl(bool* followRedirects) { *followRedirects = Never; }
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698461217


   https://issues.apache.org/jira/browse/ARROW-10085


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#discussion_r494492980



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       I hadn't thought of that. WIll try it out, thank you.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#discussion_r494551627



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       Seems to work!




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698492518


   cc @nealrichardson 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698479206


   CI failures are unrelated.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#discussion_r494460508



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       I couldn't think of anything better. @bkietz
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] bkietz commented on a change in pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#discussion_r494491830



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       Maybe a little better?
   ```suggestion
   // In AWS SDK < 1.8, Aws::Client::ClientConfiguration::followRedirects is a bool.
   void DisableRedirectsImpl(bool* followRedirects) { *followRedirects = false; }
   
   // In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
   template <typename PolicyEnum, PolicyEnum Never = PolicyEnum::NEVER>
   void DisableRedirectsImpl(PolicyEnum* followRedirects) { *followRedirects = Never; }
   
   void DisableRedirects(Config* c) {
     DisableRedirectsImpl(&c->followRedirects);
   }
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698500867


   Revision: e199de165efb6b388e422364718bd1342c4fd868
   
   Submitted crossbow builds: [ursa-labs/crossbow @ actions-553](https://github.com/ursa-labs/crossbow/branches/all?query=actions-553)
   
   |Task|Status|
   |----|------|
   |homebrew-r-autobrew|[![TravisCI](https://img.shields.io/travis/ursa-labs/crossbow/actions-553-travis-homebrew-r-autobrew.svg)](https://travis-ci.org/ursa-labs/crossbow/branches)|


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#discussion_r494460508



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       I couldn't think of anything better. @bkietz
   

##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       I hadn't thought of that. WIll try it out, thank you.

##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       Unfortunately:
   ```
   ../src/arrow/filesystem/s3fs.cc:463:6: error: unused function 'DisableRedirectsImpl' [-Werror,-Wunused-function]
   void DisableRedirectsImpl(bool* followRedirects) { *followRedirects = false; }
        ^
   ```

##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       `if constexpr` would have been nice...

##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       Seems to work!




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#discussion_r494494733



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -459,6 +459,24 @@ class S3Client : public Aws::S3::S3Client {
   }
 };
 
+// In AWS SDK < 1.8, Aws::Client::ClientConfiguration is a bool.
+// In AWS SDK >= 1.8, it's a Aws::Client::FollowRedirectsPolicy scoped enum.
+static constexpr bool HaveLegacyFollowRedirects =
+    std::is_same<bool,
+                 decltype(Aws::Client::ClientConfiguration().followRedirects)>::value;
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = false;
+}
+
+template <typename Config = Aws::Client::ClientConfiguration>
+void DisableRedirects(
+    typename std::enable_if<!HaveLegacyFollowRedirects, Config*>::type config) {
+  config->followRedirects = decltype(config->followRedirects)::NEVER;
+}
+

Review comment:
       Unfortunately:
   ```
   ../src/arrow/filesystem/s3fs.cc:463:6: error: unused function 'DisableRedirectsImpl' [-Werror,-Wunused-function]
   void DisableRedirectsImpl(bool* followRedirects) { *followRedirects = false; }
        ^
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #8261: ARROW-10085: [C++] Fix S3 region resolution on Windows

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8261:
URL: https://github.com/apache/arrow/pull/8261#issuecomment-698461217






----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org