Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-41470: [C++] Reuse deduplication logic for direct registration #41466

Merged

Conversation

bkietz
Copy link
Member

@bkietz bkietz commented Apr 30, 2024

Rationale for this change

As observed in #41309 a crossbow job on mac is failing due to duplicate registration of a factory for the file:// scheme

What changes are included in this PR?

Deduplication of registered filesystem factories is applied to direct registration as well as when merging registries.

Are these changes tested?

No, we just need to verify that the problematic crossbow job is repaired.

Are there any user-facing changes?

No

@bkietz bkietz self-assigned this Apr 30, 2024
Copy link

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:

@apache apache deleted a comment from github-actions bot Apr 30, 2024
@apache apache deleted a comment from github-actions bot Apr 30, 2024
@bkietz
Copy link
Member Author

bkietz commented Apr 30, 2024

@github-actions crossbow submit -g homebrew

Copy link

Revision: e798267

Submitted crossbow builds: ursacomputing/crossbow @ actions-05f8f5ff1f

Task Status
homebrew-cpp GitHub Actions

@vibhatha
Copy link
Collaborator

@github-actions crossbow submit java

@bkietz
Copy link
Member Author

bkietz commented Apr 30, 2024

@github-actions crossbow submit -g java-jars

Copy link

Invalid group(s) {'java-jars'}. Must be one of {'homebrew', 'verify-rc-wheels', 'conda', 'c-glib', 'verify-rc-source', 'ruby', 'integration', 'wheel', 'fuzz', 'nightly-packaging', 'linux-arm64', 'java', 'vcpkg', 'test', 'verify-rc-source-macos', 'verify-rc-jars', 'packaging', 'nightly-release', 'linux-amd64', 'example-python', 'verify-rc-source-linux', 'go', 'linux', 'verify-rc', 'nightly-tests', 'nightly', 'conan', 'example', 'example-cpp', 'r', 'cpp', 'python', 'verify-rc-binaries'}
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/8897392124

Copy link

Revision: e798267

Submitted crossbow builds: ursacomputing/crossbow @ actions-9c4f4373f4

Task Status
java-jars GitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-java-macos-amd64 GitHub Actions

@bkietz bkietz changed the title reuse deduplication logic for direct registration GH-41470: [C++] Reuse deduplication logic for direct registration Apr 30, 2024
@bkietz bkietz marked this pull request as ready for review April 30, 2024 18:08
@bkietz bkietz requested a review from lidavidm April 30, 2024 18:08
@lidavidm
Copy link
Member

lidavidm commented May 1, 2024

@github-actions crossbow submit java-jars

@@ -80,7 +80,7 @@ jobs:
fail-fast: false
matrix:
platform:
- { runs_on: ["macos-latest"], arch: "x86_64"}
- { runs_on: ["macos-13"], arch: "x86_64"}
Copy link
Member

Choose a reason for hiding this comment

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

macos-latest == macos-14 now so if we want amd64 we have to explicitly request macos-13

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels May 1, 2024
Copy link

github-actions bot commented May 1, 2024

Revision: 2fd3fcc

Submitted crossbow builds: ursacomputing/crossbow @ actions-319edba519

Task Status
java-jars GitHub Actions

@bkietz
Copy link
Member Author

bkietz commented May 1, 2024

The error doesn't seem related to filesystem anymore: https://github.com/ursacomputing/crossbow/actions/runs/8903205653/job/24451424677#step:6:16247

I'm not sure what it means though

@vibhatha
Copy link
Collaborator

vibhatha commented May 1, 2024

@lidavidm this seems to be from the recent maven build update?

@lidavidm
Copy link
Member

lidavidm commented May 1, 2024

We can merge this regardless and @vibhatha can file a new ticket.

@lidavidm lidavidm merged commit 14c54bb into apache:main May 1, 2024
37 of 38 checks passed
Copy link

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

There were no benchmark performance regressions. 🎉

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

@bkietz bkietz deleted the also-deduplicated-direct-filesystem-registration branch May 2, 2024 14:32
tolleybot pushed a commit to tmct/arrow that referenced this pull request May 2, 2024
…on (apache#41466)

### Rationale for this change

As observed in apache#41309 a crossbow job on mac is failing due to duplicate registration of a factory for the file:// scheme

### What changes are included in this PR?

Deduplication of registered filesystem factories is applied to direct registration as well as when merging registries.

### Are these changes tested?

No, we just need to verify that the problematic crossbow job is repaired.

### Are there any user-facing changes?

No

* GitHub Issue: apache#41470

Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
tolleybot pushed a commit to tmct/arrow that referenced this pull request May 4, 2024
…on (apache#41466)

### Rationale for this change

As observed in apache#41309 a crossbow job on mac is failing due to duplicate registration of a factory for the file:// scheme

### What changes are included in this PR?

Deduplication of registered filesystem factories is applied to direct registration as well as when merging registries.

### Are these changes tested?

No, we just need to verify that the problematic crossbow job is repaired.

### Are there any user-facing changes?

No

* GitHub Issue: apache#41470

Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
@raulcd
Copy link
Member

raulcd commented May 7, 2024

I will have to manually include: 2fd3fcc into 16.1.0 in order to have a passing java-jars job as we don't want to add the other commits of this PR (the filesystems split should go into 17.0.0 instead of 16.1.0)
I plan to add the above as a manual commit along with the fix for: #41578

rok pushed a commit to tmct/arrow that referenced this pull request May 8, 2024
…on (apache#41466)

### Rationale for this change

As observed in apache#41309 a crossbow job on mac is failing due to duplicate registration of a factory for the file:// scheme

### What changes are included in this PR?

Deduplication of registered filesystem factories is applied to direct registration as well as when merging registries.

### Are these changes tested?

No, we just need to verify that the problematic crossbow job is repaired.

### Are there any user-facing changes?

No

* GitHub Issue: apache#41470

Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
rok pushed a commit to tmct/arrow that referenced this pull request May 8, 2024
…on (apache#41466)

### Rationale for this change

As observed in apache#41309 a crossbow job on mac is failing due to duplicate registration of a factory for the file:// scheme

### What changes are included in this PR?

Deduplication of registered filesystem factories is applied to direct registration as well as when merging registries.

### Are these changes tested?

No, we just need to verify that the problematic crossbow job is repaired.

### Are there any user-facing changes?

No

* GitHub Issue: apache#41470

Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
…on (apache#41466)

### Rationale for this change

As observed in apache#41309 a crossbow job on mac is failing due to duplicate registration of a factory for the file:// scheme

### What changes are included in this PR?

Deduplication of registered filesystem factories is applied to direct registration as well as when merging registries.

### Are these changes tested?

No, we just need to verify that the problematic crossbow job is repaired.

### Are there any user-facing changes?

No

* GitHub Issue: apache#41470

Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
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.

None yet

4 participants