Skip to content

Conversation

@Tom-Newton
Copy link
Contributor

@Tom-Newton Tom-Newton commented Aug 1, 2023

Rationale for this change

We need to write tests for #18014. azurite is like a fake Azure blob storage so it can be used to write integration tests

What changes are included in this PR?

Extract the azurite related changes from #12914 to create a smaller PR that's easier to review. I have made very minimal changes compared to that PR.

Currently azurite is configured for all the environments where ARROW_AZURE was enabled by #35701. I assume its deliberate that its not enabled yet for windows, alpine, conda, debian or fedora builds.

Are these changes tested?

Its tested by there aren't really any good tests in this PR. I used this azurite config in #36835 to make an integration test that uses the Azure C++ SDK. On its own we can't really write tests for this azurite setup PR.

Are there any user-facing changes?

No

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@Tom-Newton Tom-Newton force-pushed the tomnewton/configure_azurite/GH-36886 branch from 8c5060f to d1102fc Compare August 1, 2023 22:53
@Tom-Newton Tom-Newton changed the title Tomnewton/configure azurite/gh 36886 GH-36886: [C++] Configure azurite in preparation for testing Azure C++ filesystem Aug 1, 2023
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

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

@Tom-Newton Tom-Newton marked this pull request as ready for review August 4, 2023 16:32
// under the License.

#include "arrow/filesystem/azurefs.h"
#include <boost/process.hpp>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use

#include <algorithm> // Missing include in boost/process
#define BOOST_NO_CXX98_FUNCTION_BASE // ARROW-17805
// This boost/asio/io_context.hpp include is needless for no MinGW
// build.
//
// This is for including boost/asio/detail/socket_types.hpp before any
// "#include <windows.h>". boost/asio/detail/socket_types.hpp doesn't
// work if windows.h is already included. boost/process.h ->
// boost/process/args.hpp -> boost/process/detail/basic_cmd.hpp
// includes windows.h. boost/process/args.hpp is included before
// boost/process/async.h that includes
// boost/asio/detail/socket_types.hpp implicitly is included.
#include <boost/asio/io_context.hpp>
// We need BOOST_USE_WINDOWS_H definition with MinGW when we use
// boost/process.hpp. See BOOST_USE_WINDOWS_H=1 in
// cpp/cmake_modules/ThirdpartyToolchain.cmake for details.
#include <boost/process.hpp>
?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason not to

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Aug 7, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 7, 2023
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@kou kou merged commit f055f5e into apache:main Aug 7, 2023
@kou kou removed the awaiting change review Awaiting change review label Aug 7, 2023
@github-actions github-actions bot added the awaiting merge Awaiting merge label Aug 7, 2023
@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

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

loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…Azure C++ filesystem (apache#36988)

### Rationale for this change
We need to write tests for apache#18014. azurite is like a fake Azure blob storage so it can be used to write integration tests

### What changes are included in this PR?
Extract the `azurite` related changes from apache#12914 to create a smaller PR that's easier to review. I have made very minimal changes compared to that PR. 

Currently `azurite` is configured for all the environments where `ARROW_AZURE` was enabled by apache#35701. I assume its deliberate that its not enabled yet for windows, alpine, conda, debian or fedora builds. 

### Are these changes tested?
Its tested by there aren't really any good tests in this PR. I used this `azurite` config in apache#36835 to make an integration test that uses the Azure C++ SDK. On its own we can't really write tests for this `azurite` setup PR. 

### Are there any user-facing changes?
No

* Closes: apache#36886

Lead-authored-by: Thomas Newton <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] Configure azurite in preparation for testing Azure C++ filesystem

2 participants