-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
mobile: first pass at hasData up calls #34180
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
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.
Is it possible to write a test for this?
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 looks great, thanks Alyssa!
@@ -14,6 +14,8 @@ | |||
#include "library/common/stream_info/extra_stream_info.h" | |||
#include "library/common/system/system_helper.h" | |||
|
|||
ABSL_FLAG(bool, envoy_reloadable_features_report_available_data, false, ""); // NOLINT |
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.
is this to avoid declaring the flag in runtime_features.cc?
// data from a server. | ||
data_available_timer_ = | ||
http_client_.dispatcher_.createTimerPostDrain([this]() { reportDataAvailable(); }); | ||
data_available_timer_->enableTimer(std::chrono::seconds(1), nullptr); |
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.
should this be something like 5ms, so we know if we're immediately blocked on data?
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.report_available_data")) { | ||
// Drain must have been called for us to get to the point we've received | ||
// data from a server. | ||
data_available_timer_ = |
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.
presumably, by (re-)creating the data_available_timer_, we delete the old one, thereby cancelling the timer enabled.
@@ -293,6 +312,7 @@ void Client::DirectStreamCallbacks::onComplete() { | |||
if (elapsed > SlowCallbackWarningThreshold) { | |||
ENVOY_LOG_EVENT(warn, "slow_on_complete_cb", "{}ms", elapsed.count()); | |||
} | |||
data_available_timer_.reset(); |
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 think we'll also want to disable this timer if we get a onError ,onCancel or endStream=true
going to close this off for now as we think we've found our stall |
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]