Copilot commented on code in PR #60622:
URL: https://github.com/apache/doris/pull/60622#discussion_r2782096204
##########
be/src/olap/rowset/rowset_meta.h:
##########
@@ -455,6 +455,13 @@ class RowsetMeta : public MetadataAdder<RowsetMeta> {
std::string debug_string() const { return
_rowset_meta_pb.ShortDebugString(); }
+ // Pre-set the encryption algorithm to avoid re-entrant get_tablet calls
+ // that can cause SingleFlight deadlock during tablet loading.
+ void set_encryption_algorithm(EncryptionAlgorithmPB algorithm) {
+ _determine_encryption_once.call(
+ [algorithm]() -> Result<EncryptionAlgorithmPB> { return
algorithm; });
+ }
+
Review Comment:
`RowsetMeta::set_encryption_algorithm()` is implemented via
`DorisCallOnce::call()`, so it will not overwrite an already-initialized (or
already-failed) `_determine_encryption_once` value. The name "set" suggests it
always updates the stored algorithm, which isn’t true and can mislead future
callers. Consider renaming to something like `preset_encryption_algorithm` (or
returning a bool indicating whether it was applied), and/or asserting that any
already-stored value matches `algorithm` to prevent silent inconsistency.
```suggestion
// Returns true if this call actually sets the algorithm, false if it
was already determined.
bool preset_encryption_algorithm(EncryptionAlgorithmPB algorithm) {
bool applied = false;
_determine_encryption_once.call(
[algorithm, &applied]() -> Result<EncryptionAlgorithmPB> {
applied = true;
return algorithm;
});
return applied;
}
// Backward-compatible wrapper; prefer preset_encryption_algorithm in
new code.
void set_encryption_algorithm(EncryptionAlgorithmPB algorithm) {
(void)preset_encryption_algorithm(algorithm);
}
```
##########
be/src/cloud/cloud_tablet.cpp:
##########
@@ -1599,6 +1599,11 @@ void CloudTablet::_submit_segment_download_task(const
RowsetSharedPtr& rs,
// RowsetMeta::fs() wraps the underlying FileSystem with PackedFileSystem
when
// packed_slice_locations is not empty, which correctly maps segment file
paths
// to their actual locations within packed files.
+ //
+ // Pre-set encryption algorithm to avoid re-entrant get_tablet() call
inside
+ // RowsetMeta::fs() which causes SingleFlight deadlock when the tablet is
not
+ // yet cached (during initial load_tablet).
+
rowset_meta->set_encryption_algorithm(_tablet_meta->encryption_algorithm());
auto file_system = rowset_meta->fs();
Review Comment:
This pre-set is only done in `_submit_segment_download_task()`, but
`_add_rowsets_directly()` can also submit inverted-index warmup tasks. When
`config::file_cache_enable_only_warm_up_idx` is enabled (segments skipped),
`_submit_inverted_index_download_task()` will still call `rowset_meta->fs()`
without the pre-set and can re-enter `ExecEnv::get_tablet()` during initial
load, so the SingleFlight deadlock can still occur. Apply the same pre-set for
the inverted-index download path too (or centralize it before any warmup task
calls `RowsetMeta::fs()`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
