-
Notifications
You must be signed in to change notification settings - Fork 459
feat(storage): support pre-warming read ranges in AsyncConnection #16275
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| #include "google/cloud/storage/internal/async/handle_redirect_error.h" | ||
| #include "google/cloud/storage/internal/async/multi_stream_manager.h" | ||
| #include "google/cloud/storage/internal/async/object_descriptor_reader_tracing.h" | ||
| #include "google/cloud/storage/internal/async/options.h" | ||
| #include "google/cloud/storage/internal/grpc/object_metadata_parser.h" | ||
| #include "google/cloud/storage/internal/hash_function.h" | ||
| #include "google/cloud/storage/internal/hash_function_impl.h" | ||
|
|
@@ -28,6 +29,7 @@ | |
| #include "google/cloud/grpc_error_delegate.h" | ||
| #include "google/cloud/internal/opentelemetry.h" | ||
| #include "google/rpc/status.pb.h" | ||
| #include <algorithm> | ||
| #include <limits> | ||
| #include <memory> | ||
| #include <utility> | ||
|
|
@@ -52,6 +54,34 @@ ObjectDescriptorImpl::ObjectDescriptorImpl( | |
| []() -> std::shared_ptr<ReadStream> { return nullptr; }, // NOLINT | ||
| std::make_shared<ReadStream>(std::move(stream), | ||
| resume_policy_prototype_->clone())); | ||
| // If pre-warmed ranges are specified, initialize their `ReadRange` objects, | ||
| // register them as active on the initial stream, and cache them. | ||
| if (options_.has<ReadRangesOption>()) { | ||
| auto const& ranges = options_.get<ReadRangesOption>(); | ||
| auto it = stream_manager_->GetFirstStream(); | ||
| if (it != stream_manager_->End()) { | ||
| std::int64_t id = 0; | ||
| std::set<std::pair<std::int64_t, std::int64_t>> seen_ranges; | ||
| for (auto const& r : ranges) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||
| auto range_key = std::make_pair(r.offset, r.length); | ||
| if (!seen_ranges.insert(range_key).second) { | ||
| continue; // Skip duplicate range. | ||
| } | ||
| ++id; | ||
| auto range = std::make_shared<ReadRange>(r.offset, r.length, | ||
| read_object_spec_.bucket(), | ||
| read_object_spec_.object()); | ||
| // Registering on the stream allows `OnRead` to route incoming data to | ||
| // these ranges. | ||
| it->active_ranges.emplace(id, range); | ||
| // Cache them so subsequent `Read()` calls can claim them. | ||
| prewarmed_ranges_.emplace(range_key, PrewarmedRange{range, id}); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would happen if the user configures pre-warmed ranges but their application never actually calls Read() to claim them? |
||
| } | ||
| // Ensure new dynamically requested ranges use IDs that don't conflict | ||
| // with pre-warmed ones. | ||
| read_id_generator_ = id; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ObjectDescriptorImpl::~ObjectDescriptorImpl() { Cancel(); } | ||
|
|
@@ -193,6 +223,21 @@ std::unique_ptr<storage::AsyncReaderConnection> ObjectDescriptorImpl::Read( | |
| read_object_spec_.bucket(), read_object_spec_.object()); | ||
|
|
||
| std::unique_lock<std::mutex> lk(mu_); | ||
| // Check if this range matches a pre-warmed range. | ||
| auto cache_key = std::make_pair(p.start, p.length); | ||
| auto cache_it = prewarmed_ranges_.find(cache_key); | ||
| if (cache_it != prewarmed_ranges_.end()) { | ||
| // Cache hit. Claim the pre-warmed range and return it to the user. | ||
| auto prewarmed = std::move(cache_it->second); | ||
| prewarmed_ranges_.erase(cache_it); | ||
| lk.unlock(); | ||
| if (!internal::TracingEnabled(options_)) { | ||
| return std::unique_ptr<storage::AsyncReaderConnection>( | ||
| std::make_unique<ObjectDescriptorReader>(std::move(prewarmed.range))); | ||
| } | ||
| return MakeTracingObjectDescriptorReader(std::move(prewarmed.range)); | ||
| } | ||
|
|
||
| if (stream_manager_->Empty()) { | ||
| lk.unlock(); | ||
| range->OnFinish(Status(StatusCode::kFailedPrecondition, | ||
|
|
@@ -366,9 +411,17 @@ void ObjectDescriptorImpl::OnRead( | |
| auto id = range_data.read_range().read_id(); | ||
| auto const l = copy.find(id); | ||
| if (l == copy.end()) continue; | ||
| // TODO(#15104) - Consider returning if the range is done, and then | ||
| // skipping CleanupDoneRanges(). | ||
| l->second->OnRead(std::move(range_data), is_transcoded, object_size); | ||
|
|
||
| auto range = l->second; | ||
| bool active = false; | ||
| lk.lock(); | ||
| active = it->active_ranges.count(id) != 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. QQ: why do we need to do this? |
||
| lk.unlock(); | ||
| if (active) { | ||
| // TODO(#15104) - Consider returning if the range is done, and then | ||
| // skipping CleanupDoneRanges(). | ||
| range->OnRead(std::move(range_data), is_transcoded, object_size); | ||
| } | ||
| } | ||
| lk.lock(); | ||
| stream_manager_->CleanupDoneRanges(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.
This exact same ID generation and deduplication loop is copied in both connection_impl.cc and object_descriptor_impl.cc. If someone updates the logic in one file and forgets the other, the read_ids won't match and data will be sent to the wrong ranges. Should we move this logic into a single shared helper function?