Skip to content
New issue

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

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

Already on GitHub? # to your account

Add make_dirs and construct_data_paths to sink.cpp #47

Merged
merged 7 commits into from
Jan 31, 2025

Conversation

aliddell
Copy link
Member

@aliddell aliddell commented Jan 22, 2025

Moving away from the SinkCreator class. Depends on #45.

@aliddell aliddell changed the title Add make_dirs and construct_data_paths to zarr.common. Add make_dirs and construct_data_paths to sink.cpp Jan 22, 2025
@aliddell aliddell force-pushed the common-construct-data-paths branch from 6b4ec7f to d072255 Compare January 22, 2025 17:54
@aliddell aliddell force-pushed the common-construct-data-paths branch from d072255 to f034d62 Compare January 22, 2025 19:07
@aliddell aliddell mentioned this pull request Jan 22, 2025
Base automatically changed from remove-arraywriter-version to main January 23, 2025 19:32
@aliddell aliddell requested a review from jeskesen January 23, 2025 19:36
@aliddell aliddell marked this pull request as ready for review January 23, 2025 19:36
Copy link
Member

@nclack nclack left a comment

Choose a reason for hiding this comment

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

I had a couple questions. One thing that might help me for these prs is to point out where you want review - I suspect I'm commenting/asking about things that have already been discussed in the past. Nothing major so approving.

src/streaming/sink.cpp Show resolved Hide resolved

latch.count_down();
all_successful.fetch_and(static_cast<char>(success));
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, you can use release memory ordering here and acquire on the final read.

The default (cst) should work fine and I would be surprised if it mattered for performance.

src/streaming/sink.cpp Show resolved Hide resolved
@aliddell aliddell merged commit 9a2bb90 into main Jan 31, 2025
7 checks passed
@aliddell aliddell deleted the common-construct-data-paths branch January 31, 2025 16:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants