Add FileSystemBuffer::Fragment class to support file_system_buffer#20949
Add FileSystemBuffer::Fragment class to support file_system_buffer#20949mattklein123 merged 8 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/assign mattklein123 |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/retest |
|
Retrying Azure Pipelines: |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Nice. Generally LGTM with some small comments. Thank you!
/wait
|
|
||
| // Removes the buffer from a memory instance and resets it to size 0. | ||
| // | ||
| // It is an error to call extract() on a Fragment that is not in memory. |
There was a problem hiding this comment.
Presumably this returns nullptr? Should this return StatusOr to make precondition failures / programming errors more obvious?
There was a problem hiding this comment.
Expanded on the comment to clarify it will cause an exception to call extract on a fragment that is not in memory.
| // [this](absl::StatusOr<UpdateFragmentFunction> status_or_update_fragment) { | ||
| // if (status_or_update_fragment.ok()) { | ||
| // auto update_fragment = status_or_update_fragment.value(); | ||
| // dispatcher->dispatch([this, update_fragment]() { |
There was a problem hiding this comment.
Where does dispatcher come from in this example? Related to our convo on Slack, this can likely also be gone by the time dispatcher runs. Should some kind of weak_ptr callback system be built into this thing so the caller doesn't need to bother? Or can we at least document more how this is supposed to be used?
There was a problem hiding this comment.
I was trying to keep this helper more context-agnostic, but you're right, it is simpler if the helper gets a dispatch function directly. I still offloaded the safety aspect to the caller, because that way Fragment doesn't need to know anything about filters, but taking a dispatch function parameter makes it much simpler than a callback that returns a callback that must be called back!
| Fragment::toStorage(AsyncFileHandle file, off_t offset, | ||
| std::function<void(absl::StatusOr<UpdateFragmentFunction>)> on_done) { | ||
| ASSERT(isMemory()); | ||
| auto data = dynamic_cast<MemoryFragment*>(data_.get())->extract(); |
There was a problem hiding this comment.
Instead of using dynamic_cast to deal with the state machine, and potential errors show up as nullptr, can you use absl::Variant instead? Then the state machine and possible values are more clear?
There was a problem hiding this comment.
Nice suggestion, thanks. Not so keen on more internal implementation details being moved into the header file, which unfortunately is required for variant, but it's better enough to be worth it.
…equiring caller callback call a callback Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
Can you check tidy CI? /wait |
…convert unique_ptr to shared_ptr to satisfy lambda (!) Signed-off-by: Raven Black <ravenblack@dropbox.com>
Head branch was pushed to by a user without write access
Signed-off-by: Raven Black <ravenblack@dropbox.com>
…nvoyproxy#20949) Signed-off-by: Raven Black <ravenblack@dropbox.com>
2 participants
Signed-off-by: Raven Black ravenblack@dropbox.com
Commit Message: Add FileSystemBuffer::Fragment class to support file_system_buffer
Additional Description: This is a small helper class that manages pieces of stream buffer, moving them to disk/SSD and back to memory asynchronously on demand, with completion callback. Build rule is envoy_cc_library for now so the eventual adding the filter to the build step can be done separately from the implementation.
Risk Level: None yet, not in use.
Testing: 100% unit test coverage. Integration test for the filter in a later PR will also use it.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a