GH-39292 [C++][FS]: Remove the AzureBackend enum and add more flexible connection options by felipecrv · Pull Request #39293 · apache/arrow

felipecrv · 2023-12-19T13:16:09Z

Rationale for this change

It's good to avoid mentioning the specific test environment in the implementation code.

What changes are included in this PR?

Removal of the enum
Removal of the AzureOptions::backend class member
Addition of more options to AzureOptions
Removal of some private string members of AzureOptions -- the URLs are built on-the-fly when the clients are instantiated now

Are these changes tested?

Yes.

Are there any user-facing changes?

Changes to the public interface (azurefs.h) that won't affect users because the AzureFS implementation is not used yet.

Closes: [C++] Remove the AzureOptions::backend option #39292

… to AzureOptions

felipecrv · 2023-12-19T13:16:23Z

@Tom-Newton @kou

github-actions · 2023-12-19T13:16:39Z

⚠️ GitHub issue #39292has been automatically assigned in GitHub to PR creator.

felipecrv · 2023-12-19T13:59:20Z

Unrelated failures on macOS and appveyor builds.

kou · 2023-12-19T20:26:41Z

+ const std::string& account_name) {
+ std::string url;
+ url += scheme + "://";
+ if (!authority.empty()) {


I think that we should reject empty authority because it builds no host part URL.

My reasoning here is that there are many validations I could apply, but the most complete validation will come from the error messages triggered by using the filesystem with a bogus URL with 3 slashes after https:.

conbench-apache-arrow · 2023-12-20T07:13:59Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 56991d3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…lexible connection options (apache#39293) ### Rationale for this change It's good to avoid mentioning the specific test environment in the implementation code. ### What changes are included in this PR? - Removal of the enum - Removal of the `AzureOptions::backend` class member - Addition of more options to `AzureOptions` - Removal of some private string members of `AzureOptions` -- the URLs are built on-the-fly when the clients are instantiated now ### Are these changes tested? Yes. ### Are there any user-facing changes? Changes to the public interface (`azurefs.h`) that won't affect users because the `AzureFS` implementation is not used yet. * Closes: apache#39292 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2023-12-19T13:16:09Z

Rationale for this change

It's good to avoid mentioning the specific test environment in the implementation code.

What changes are included in this PR?

Removal of the enum
Removal of the AzureOptions::backend class member
Addition of more options to AzureOptions
Removal of some private string members of AzureOptions -- the URLs are built on-the-fly when the clients are instantiated now

Are these changes tested?

Yes.

Are there any user-facing changes?

Changes to the public interface (azurefs.h) that won't affect users because the AzureFS implementation is not used yet.

Closes: [C++] Remove the AzureOptions::backend option #39292

… to AzureOptions

felipecrv · 2023-12-19T13:16:23Z

@Tom-Newton @kou

github-actions · 2023-12-19T13:16:39Z

⚠️ GitHub issue #39292has been automatically assigned in GitHub to PR creator.

felipecrv · 2023-12-19T13:59:20Z

Unrelated failures on macOS and appveyor builds.

kou · 2023-12-19T20:26:41Z

+ const std::string& account_name) {
+ std::string url;
+ url += scheme + "://";
+ if (!authority.empty()) {


I think that we should reject empty authority because it builds no host part URL.

My reasoning here is that there are many validations I could apply, but the most complete validation will come from the error messages triggered by using the filesystem with a bogus URL with 3 slashes after https:.

conbench-apache-arrow · 2023-12-20T07:13:59Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 56991d3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…lexible connection options (apache#39293) ### Rationale for this change It's good to avoid mentioning the specific test environment in the implementation code. ### What changes are included in this PR? - Removal of the enum - Removal of the `AzureOptions::backend` class member - Addition of more options to `AzureOptions` - Removal of some private string members of `AzureOptions` -- the URLs are built on-the-fly when the clients are instantiated now ### Are these changes tested? Yes. ### Are there any user-facing changes? Changes to the public interface (`azurefs.h`) that won't affect users because the `AzureFS` implementation is not used yet. * Closes: apache#39292 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2023-12-19T13:16:09Z

Rationale for this change

It's good to avoid mentioning the specific test environment in the implementation code.

What changes are included in this PR?

Removal of the enum
Removal of the AzureOptions::backend class member
Addition of more options to AzureOptions
Removal of some private string members of AzureOptions -- the URLs are built on-the-fly when the clients are instantiated now

Are these changes tested?

Yes.

Are there any user-facing changes?

Changes to the public interface (azurefs.h) that won't affect users because the AzureFS implementation is not used yet.

Closes: [C++] Remove the AzureOptions::backend option #39292

… to AzureOptions

felipecrv · 2023-12-19T13:16:23Z

@Tom-Newton @kou

github-actions · 2023-12-19T13:16:39Z

⚠️ GitHub issue #39292has been automatically assigned in GitHub to PR creator.

felipecrv · 2023-12-19T13:59:20Z

Unrelated failures on macOS and appveyor builds.

kou · 2023-12-19T20:26:41Z

+ const std::string& account_name) {
+ std::string url;
+ url += scheme + "://";
+ if (!authority.empty()) {


I think that we should reject empty authority because it builds no host part URL.

My reasoning here is that there are many validations I could apply, but the most complete validation will come from the error messages triggered by using the filesystem with a bogus URL with 3 slashes after https:.

conbench-apache-arrow · 2023-12-20T07:13:59Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 56991d3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…lexible connection options (apache#39293) ### Rationale for this change It's good to avoid mentioning the specific test environment in the implementation code. ### What changes are included in this PR? - Removal of the enum - Removal of the `AzureOptions::backend` class member - Addition of more options to `AzureOptions` - Removal of some private string members of `AzureOptions` -- the URLs are built on-the-fly when the clients are instantiated now ### Are these changes tested? Yes. ### Are there any user-facing changes? Changes to the public interface (`azurefs.h`) that won't affect users because the `AzureFS` implementation is not used yet. * Closes: apache#39292 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2023-12-19T13:16:09Z

Rationale for this change

It's good to avoid mentioning the specific test environment in the implementation code.

What changes are included in this PR?

Removal of the enum
Removal of the AzureOptions::backend class member
Addition of more options to AzureOptions
Removal of some private string members of AzureOptions -- the URLs are built on-the-fly when the clients are instantiated now

Are these changes tested?

Yes.

Are there any user-facing changes?

Changes to the public interface (azurefs.h) that won't affect users because the AzureFS implementation is not used yet.

Closes: [C++] Remove the AzureOptions::backend option #39292

… to AzureOptions

felipecrv · 2023-12-19T13:16:23Z

@Tom-Newton @kou

github-actions · 2023-12-19T13:16:39Z

⚠️ GitHub issue #39292has been automatically assigned in GitHub to PR creator.

felipecrv · 2023-12-19T13:59:20Z

Unrelated failures on macOS and appveyor builds.

kou · 2023-12-19T20:26:41Z

+ const std::string& account_name) {
+ std::string url;
+ url += scheme + "://";
+ if (!authority.empty()) {


I think that we should reject empty authority because it builds no host part URL.

My reasoning here is that there are many validations I could apply, but the most complete validation will come from the error messages triggered by using the filesystem with a bogus URL with 3 slashes after https:.

conbench-apache-arrow · 2023-12-20T07:13:59Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 56991d3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…lexible connection options (apache#39293) ### Rationale for this change It's good to avoid mentioning the specific test environment in the implementation code. ### What changes are included in this PR? - Removal of the enum - Removal of the `AzureOptions::backend` class member - Addition of more options to `AzureOptions` - Removal of some private string members of `AzureOptions` -- the URLs are built on-the-fly when the clients are instantiated now ### Are these changes tested? Yes. ### Are there any user-facing changes? Changes to the public interface (`azurefs.h`) that won't affect users because the `AzureFS` implementation is not used yet. * Closes: apache#39292 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2023-12-19T13:16:09Z

Rationale for this change

It's good to avoid mentioning the specific test environment in the implementation code.

What changes are included in this PR?

Removal of the enum
Removal of the AzureOptions::backend class member
Addition of more options to AzureOptions
Removal of some private string members of AzureOptions -- the URLs are built on-the-fly when the clients are instantiated now

Are these changes tested?

Yes.

Are there any user-facing changes?

Changes to the public interface (azurefs.h) that won't affect users because the AzureFS implementation is not used yet.

Closes: [C++] Remove the AzureOptions::backend option #39292

… to AzureOptions

felipecrv · 2023-12-19T13:16:23Z

@Tom-Newton @kou

github-actions · 2023-12-19T13:16:39Z

⚠️ GitHub issue #39292has been automatically assigned in GitHub to PR creator.

felipecrv · 2023-12-19T13:59:20Z

Unrelated failures on macOS and appveyor builds.

kou · 2023-12-19T20:26:41Z

+ const std::string& account_name) {
+ std::string url;
+ url += scheme + "://";
+ if (!authority.empty()) {


I think that we should reject empty authority because it builds no host part URL.

My reasoning here is that there are many validations I could apply, but the most complete validation will come from the error messages triggered by using the filesystem with a bogus URL with 3 slashes after https:.

conbench-apache-arrow · 2023-12-20T07:13:59Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 56991d3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…lexible connection options (apache#39293) ### Rationale for this change It's good to avoid mentioning the specific test environment in the implementation code. ### What changes are included in this PR? - Removal of the enum - Removal of the `AzureOptions::backend` class member - Addition of more options to `AzureOptions` - Removal of some private string members of `AzureOptions` -- the URLs are built on-the-fly when the clients are instantiated now ### Are these changes tested? Yes. ### Are there any user-facing changes? Changes to the public interface (`azurefs.h`) that won't affect users because the `AzureFS` implementation is not used yet. * Closes: apache#39292 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2023-12-19T13:16:09Z

Rationale for this change

It's good to avoid mentioning the specific test environment in the implementation code.

What changes are included in this PR?

Removal of the enum
Removal of the AzureOptions::backend class member
Addition of more options to AzureOptions
Removal of some private string members of AzureOptions -- the URLs are built on-the-fly when the clients are instantiated now

Are these changes tested?

Yes.

Are there any user-facing changes?

Changes to the public interface (azurefs.h) that won't affect users because the AzureFS implementation is not used yet.

Closes: [C++] Remove the AzureOptions::backend option #39292

… to AzureOptions

felipecrv · 2023-12-19T13:16:23Z

@Tom-Newton @kou

github-actions · 2023-12-19T13:16:39Z

⚠️ GitHub issue #39292has been automatically assigned in GitHub to PR creator.

felipecrv · 2023-12-19T13:59:20Z

Unrelated failures on macOS and appveyor builds.

kou · 2023-12-19T20:26:41Z

+ const std::string& account_name) {
+ std::string url;
+ url += scheme + "://";
+ if (!authority.empty()) {


I think that we should reject empty authority because it builds no host part URL.

My reasoning here is that there are many validations I could apply, but the most complete validation will come from the error messages triggered by using the filesystem with a bogus URL with 3 slashes after https:.

conbench-apache-arrow · 2023-12-20T07:13:59Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 56991d3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…lexible connection options (apache#39293) ### Rationale for this change It's good to avoid mentioning the specific test environment in the implementation code. ### What changes are included in this PR? - Removal of the enum - Removal of the `AzureOptions::backend` class member - Addition of more options to `AzureOptions` - Removal of some private string members of `AzureOptions` -- the URLs are built on-the-fly when the clients are instantiated now ### Are these changes tested? Yes. ### Are there any user-facing changes? Changes to the public interface (`azurefs.h`) that won't affect users because the `AzureFS` implementation is not used yet. * Closes: apache#39292 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2023-12-19T13:16:09Z

Rationale for this change

It's good to avoid mentioning the specific test environment in the implementation code.

What changes are included in this PR?

Removal of the enum
Removal of the AzureOptions::backend class member
Addition of more options to AzureOptions
Removal of some private string members of AzureOptions -- the URLs are built on-the-fly when the clients are instantiated now

Are these changes tested?

Yes.

Are there any user-facing changes?

Changes to the public interface (azurefs.h) that won't affect users because the AzureFS implementation is not used yet.

Closes: [C++] Remove the AzureOptions::backend option #39292

… to AzureOptions

felipecrv · 2023-12-19T13:16:23Z

@Tom-Newton @kou

github-actions · 2023-12-19T13:16:39Z

⚠️ GitHub issue #39292has been automatically assigned in GitHub to PR creator.

felipecrv · 2023-12-19T13:59:20Z

Unrelated failures on macOS and appveyor builds.

kou · 2023-12-19T20:26:41Z

+ const std::string& account_name) {
+ std::string url;
+ url += scheme + "://";
+ if (!authority.empty()) {


I think that we should reject empty authority because it builds no host part URL.

My reasoning here is that there are many validations I could apply, but the most complete validation will come from the error messages triggered by using the filesystem with a bogus URL with 3 slashes after https:.

conbench-apache-arrow · 2023-12-20T07:13:59Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 56991d3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…lexible connection options (apache#39293) ### Rationale for this change It's good to avoid mentioning the specific test environment in the implementation code. ### What changes are included in this PR? - Removal of the enum - Removal of the `AzureOptions::backend` class member - Addition of more options to `AzureOptions` - Removal of some private string members of `AzureOptions` -- the URLs are built on-the-fly when the clients are instantiated now ### Are these changes tested? Yes. ### Are there any user-facing changes? Changes to the public interface (`azurefs.h`) that won't affect users because the `AzureFS` implementation is not used yet. * Closes: apache#39292 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2023-12-19T13:16:09Z

Rationale for this change

It's good to avoid mentioning the specific test environment in the implementation code.

What changes are included in this PR?

Removal of the enum
Removal of the AzureOptions::backend class member
Addition of more options to AzureOptions
Removal of some private string members of AzureOptions -- the URLs are built on-the-fly when the clients are instantiated now

Are these changes tested?

Yes.

Are there any user-facing changes?

Changes to the public interface (azurefs.h) that won't affect users because the AzureFS implementation is not used yet.

Closes: [C++] Remove the AzureOptions::backend option #39292

… to AzureOptions

felipecrv · 2023-12-19T13:16:23Z

@Tom-Newton @kou

github-actions · 2023-12-19T13:16:39Z

⚠️ GitHub issue #39292has been automatically assigned in GitHub to PR creator.

felipecrv · 2023-12-19T13:59:20Z

Unrelated failures on macOS and appveyor builds.

kou · 2023-12-19T20:26:41Z

+ const std::string& account_name) {
+ std::string url;
+ url += scheme + "://";
+ if (!authority.empty()) {


I think that we should reject empty authority because it builds no host part URL.

My reasoning here is that there are many validations I could apply, but the most complete validation will come from the error messages triggered by using the filesystem with a bogus URL with 3 slashes after https:.

conbench-apache-arrow · 2023-12-20T07:13:59Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 56991d3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…lexible connection options (apache#39293) ### Rationale for this change It's good to avoid mentioning the specific test environment in the implementation code. ### What changes are included in this PR? - Removal of the enum - Removal of the `AzureOptions::backend` class member - Addition of more options to `AzureOptions` - Removal of some private string members of `AzureOptions` -- the URLs are built on-the-fly when the clients are instantiated now ### Are these changes tested? Yes. ### Are there any user-facing changes? Changes to the public interface (`azurefs.h`) that won't affect users because the `AzureFS` implementation is not used yet. * Closes: apache#39292 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Remove the AzureBackend enum and add more flexible connection options…
3d5a4e2
… to AzureOptions

felipecrv requested a review from pitrou December 19, 2023 13:16

github-actionsBot added Component: C++ awaiting review Awaiting review labels Dec 19, 2023

pitrou approved these changes Dec 19, 2023
View reviewed changes

github-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 19, 2023

felipecrv merged commit 56991d3 into apache:mainDec 19, 2023

felipecrv removed the awaiting committer review Awaiting committer review label Dec 19, 2023

felipecrv deleted the azure_options_backend branch December 19, 2023 14:00

kou reviewed Dec 19, 2023
View reviewed changes

github-actionsBot added the awaiting changes Awaiting changes label Dec 19, 2023