Skip to content

Commit

Permalink
apacheGH-41470: [C++] Reuse deduplication logic for direct registrati…
Browse files Browse the repository at this point in the history
…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 <[email protected]>
Co-authored-by: David Li <[email protected]>
Signed-off-by: David Li <[email protected]>
  • Loading branch information
2 people authored and tolleybot committed May 2, 2024
1 parent d9fa916 commit f01aff3
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 6 deletions.
4 changes: 2 additions & 2 deletions cpp/src/arrow/filesystem/filesystem.cc
Original file line number Diff line number Diff line change
Expand Up @@ -761,8 +761,8 @@ class FileSystemFactoryRegistry {
RETURN_NOT_OK(CheckValid());

auto [it, success] = scheme_to_factory_.emplace(
std::move(scheme), Registered{std::move(factory), std::move(finalizer)});
if (success) {
std::move(scheme), Registered{factory, std::move(finalizer)});
if (success || (it->second.ok() && it->second->factory == factory)) {
return Status::OK();
}

Expand Down
7 changes: 4 additions & 3 deletions cpp/src/arrow/filesystem/localfs_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,16 @@ TEST(FileSystemFromUri, RuntimeRegisteredFactory) {
EXPECT_THAT(FileSystemFromUri("slowfile2:///hey/yo", &path),
Raises(StatusCode::Invalid));

EXPECT_THAT(RegisterFileSystemFactory("slowfile2", {SlowFileSystemFactory, "", 0}),
Ok());
EXPECT_THAT(
RegisterFileSystemFactory("slowfile2", {SlowFileSystemFactory, __FILE__, __LINE__}),
Ok());

ASSERT_OK_AND_ASSIGN(auto fs, FileSystemFromUri("slowfile2:///hey/yo", &path));
EXPECT_EQ(path, "/hey/yo");
EXPECT_EQ(fs->type_name(), "slow");

EXPECT_THAT(
RegisterFileSystemFactory("slowfile2", {SlowFileSystemFactory, "", 0}),
RegisterFileSystemFactory("slowfile2", {SlowFileSystemFactory, __FILE__, __LINE__}),
Raises(StatusCode::KeyError,
testing::HasSubstr("Attempted to register factory for scheme 'slowfile2' "
"but that scheme is already registered")));
Expand Down
2 changes: 1 addition & 1 deletion dev/tasks/java-jars/github.yml
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ jobs:
fail-fast: false
matrix:
platform:
- { runs_on: ["macos-latest"], arch: "x86_64"}
- { runs_on: ["macos-13"], arch: "x86_64"}
- { runs_on: ["macos-14"], arch: "aarch_64" }
env:
MACOSX_DEPLOYMENT_TARGET: "10.15"
Expand Down

0 comments on commit f01aff3

Please sign in to comment.