Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(clp-core): Add the write path for single-file archives. #646
base: main
Are you sure you want to change the base?
feat(clp-core): Add the write path for single-file archives. #646
Changes from 26 commits
a5ef64f
615fafd
b84c2da
d1ec9fe
869f1b3
d84c002
f4136f8
6bbf12e
5c75147
c1f12df
0ab6e0e
d4ed4f6
82b9802
393049b
5428403
7e261f7
0bd9b27
d207630
c2fd37d
cdafb8d
dcadf04
265b4e4
c7361c2
910e558
3ab46fd
a12ac42
4c26177
7916bb9
7fc5cb9
0ae4822
1b7d73e
7b68544
05377b8
e0fcefe
d8a3c70
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about. I feel like the second "and" after a list makes the sentence awkward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Resolve clang-format pipeline failures.
Multiple lines violate the required code style. Please run clang-format or apply the suggested changes to pass the lint checks.
#!/bin/bash clang-format -i components/core/src/clp/streaming_archive/single_file_archive/writer.cpp
Also applies to: 79-79, 88-88, 91-91, 173-173, 184-184, 189-189
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 71-71: code should be clang-formatted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now looking at the code again, wouldn't it make more sense to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about removing the function completely? I don't think we have an existing function in utils that calls stat on a closed file.
Something like
I think I originally created the function, so I could rethrow the filesystem exception as our own exception. But on second thought, I think it's fine to just throw the filesystem exception, and document it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it is ok. I remember Zhihao had a talk about error handling but I wasn't paying attention enough.
@LinZhihao-723, do you think we can directly throw the exception from std::filesystem ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made change for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering defining "" as a constant in defs.hpp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use auto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "last_segment_id" as suggested in my previous review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I missed this. last_segment_id is slightly inaccurate since it is actually an id for a segment that does not exist (it is the next segment to be written). The last_segment_id is the next_segment_id -1.
I think
num_segments
could also work. Let me know what you think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try to see if an exception is throw here, what will user see?
There are cases where the exception is caught and rethrown by some upper level caller, and as a result it's really hard to root cause the issue just by looking at the exception text. Want to avoid this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception propagates to the top which is good. If you want i can change the message from
single_file_archive operation failed
to something custom about multi-file archive not being deleted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirkrodrigues wonder if we should strictly throw an exception here, or report an warning.
Technically, the singile file archive is correctly created, we just fail to remove mulit-archive files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked at the code, but if the only issue is unnecessary files on disk, sure we could change it to a warning log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to SPDLOG_WARN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling for cleanup operation
📝 Committable suggestion