-
Notifications
You must be signed in to change notification settings - Fork 4k
Name resolution delay #12893
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
Open
AgraVator
wants to merge
46
commits into
grpc:master
Choose a base branch
from
AgraVator:name-resolution-delay
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Name resolution delay #12893
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
b50ca84
core,api,xds: Implement load balancing policy delay plumbing
AgraVator c38ce1d
fix: tests
AgraVator a992bdf
fix: minor changes
AgraVator 6a55ff2
add missing endDelay()
AgraVator 389b96f
core,api,rls,util,xds: Implement dual Load Balancer delay APIs and ca…
AgraVator 5e56f38
core: Add 100% test coverage for dual LB delay APIs and cadence rules
AgraVator 6a3572b
opentelemetry: Implement dual Load Balancer delay spans and metrics
AgraVator 9a4f21f
Merge remote-tracking branch 'upstream/master' into lb-policy-delay
AgraVator d25d064
Implement Name Resolution and unified RPC Delay Observability specifi…
AgraVator ffb485e
Ensure thread-safety and unit test coverage for Call-Level Delay APIs…
AgraVator 42352f2
Add comprehensive End-to-End tests for Call-Level Name Resolution Del…
AgraVator a76996c
Ensure Call-Level delay recording only triggers when RPCs are queued …
AgraVator 7c94743
Fix CdsLoadBalancer2Test atLeastOnce static import and assertions on …
AgraVator a7830ff
Fix PriorityLoadBalancerTest handleNameResolutionError assertion on n…
AgraVator b482c43
Fix checkstyle import ordering in OpenTelemetryTracingModuleTest
AgraVator fbbc1b9
Merge remote-tracking branch 'upstream/master' into name-resolution-d…
AgraVator df48bee
core, opentelemetry: Harden Name Resolution & LB delay state machines…
AgraVator fc9721d
api: update @since 1.82.0 to 1.84.0
AgraVator 162caef
test: remove temporary stress test files prior to PR submission
AgraVator f3bdcd9
core: align PendingStream synchronized (this) blocks with ManagedChan…
AgraVator 9cc310b
opentelemetry: add targeted unit tests to expand branch coverage for …
AgraVator 0a7eb26
core: harden PendingStream synchronization and add multithreaded race…
AgraVator 0bc1634
opentelemetry: add end-to-end Client/Server simulation tests for dela…
AgraVator 20fc8d0
Fix PR #12893 CI failures: revert PickResult.withError default delay,…
AgraVator a7398d7
cleanup: remove extra stress and unit tests, keeping only essential C…
AgraVator a9ea9e7
test: restore unit tests from master and add test coverage for delay …
AgraVator 35547a8
test: restore pickResult_withSubchannelReplacement and pickResult_wit…
AgraVator 6571c2d
test: remove multi-threaded stress tests from DelayedClientTransportT…
AgraVator b7227a7
test: remove unused imports from ManagedChannelImplTest and DelayedCl…
AgraVator 129b8e5
test(opentelemetry): add unit tests for delay metrics and tracing bra…
AgraVator ff1ed50
test(opentelemetry): remove A121DelayObservabilityWrapperTest
AgraVator d48258e
test(opentelemetry): add tests for callEnded/streamClosed guards and …
AgraVator 24cd5c3
test(opentelemetry): add tests for delay reason-changed guards withou…
AgraVator e63b9ee
test: expand branch coverage for OobChannel, delay metrics, and traci…
AgraVator 42e8686
refactor: simplify delay synchronization and telemetry style across P…
AgraVator f7216e3
core: restore state transition comments in DelayedClientTransport.upd…
AgraVator ffc15db
test, opentelemetry: address PR review feedback and add gRFC A121 nam…
AgraVator 2f71b12
test, opentelemetry: add unit tests for missing code coverage branche…
AgraVator 2416612
test, opentelemetry: fix unit test setups for checkstyle and null checks
AgraVator 6f6edf2
test: add unit tests covering missing branch combinations in DelayedC…
AgraVator 9c5406c
test: expand unit test coverage for targetAttributeFilter, clientAtte…
AgraVator e23ccfa
test: add coverage for initialType null, Baggage context, and stream …
AgraVator 6d5feab
test: add unstarted delay end tests for OpenTelemetryMetricsModule
AgraVator 3c67ba3
test: add emptyTracersAndNullInitialReason test for DelayedClientTran…
AgraVator 0309b66
style: remove unnecessary fully qualified class names across PR files
AgraVator acf4c1e
test: resolve all review discussions on delay observability branch co…
AgraVator File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -396,7 +396,7 @@ private static String determineQueuingDelayReason(@Nullable PickResult pickResul | |
| return "client channel: waiting for picker"; | ||
| } | ||
|
|
||
| private class PendingStream extends DelayedStream { | ||
| class PendingStream extends DelayedStream { | ||
| private final PickSubchannelArgs args; | ||
| private final Context context = Context.current(); | ||
| private final ClientStreamTracer[] tracers; | ||
|
|
@@ -405,6 +405,8 @@ private class PendingStream extends DelayedStream { | |
| @Nullable private String activeDelayType; | ||
| @GuardedBy("this") | ||
| @Nullable private String activeDelayReason; | ||
| @GuardedBy("this") | ||
| private boolean delayEnded; | ||
|
|
||
| private PendingStream(PickSubchannelArgs args, ClientStreamTracer[] tracers, | ||
| @Nullable String initialType, @Nullable String initialReason) { | ||
|
|
@@ -428,11 +430,11 @@ private PendingStream(PickSubchannelArgs args, ClientStreamTracer[] tracers, | |
| * structured transition event is appended to the active span without span re-creation. | ||
| */ | ||
| synchronized void updateDelay(@Nullable String newType, @Nullable String newReason) { | ||
| if (getRealStream() != null) { | ||
| if (getRealStream() != null || delayEnded) { | ||
| return; | ||
| } | ||
| // Delay type changed (e.g., from RLS lookup to connecting). End the previous delay. | ||
| if (!Objects.equals(activeDelayType, newType)) { | ||
| // Delay type changed (e.g., from RLS lookup to connecting). End the previous delay. | ||
| if (activeDelayType != null) { | ||
|
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. The is reported missing coverage for the branch when |
||
| for (ClientStreamTracer tracer : tracers) { | ||
| tracer.recordAttemptDelayEnd(); | ||
|
|
@@ -446,8 +448,9 @@ synchronized void updateDelay(@Nullable String newType, @Nullable String newReas | |
| } | ||
| } | ||
| } | ||
| if (newType != null && newReason != null && !Objects.equals(activeDelayReason, newReason)) { | ||
| // Delay type is unchanged, but the reason changed (e.g., priority failover). | ||
| // Delay type is unchanged, but the reason changed (e.g., priority failover). | ||
| if (newType != null && newReason != null | ||
| && !Objects.equals(activeDelayReason, newReason)) { | ||
| activeDelayReason = newReason; | ||
| for (ClientStreamTracer tracer : tracers) { | ||
| tracer.recordAttemptDelayReasonChanged(newReason); | ||
|
|
@@ -459,6 +462,10 @@ synchronized void updateDelay(@Nullable String newType, @Nullable String newReas | |
| * Ends active attempt delay segment telemetry upon stream creation or stream cancellation. | ||
| */ | ||
| synchronized void endDelay() { | ||
| if (delayEnded) { | ||
| return; | ||
| } | ||
| delayEnded = true; | ||
| if (activeDelayType != null) { | ||
| for (ClientStreamTracer tracer : tracers) { | ||
| tracer.recordAttemptDelayEnd(); | ||
|
|
@@ -469,8 +476,9 @@ synchronized void endDelay() { | |
| } | ||
|
|
||
| Runnable setStreamAndEndDelay(ClientStream stream) { | ||
| Runnable runnable = setStream(stream); | ||
| endDelay(); | ||
| return setStream(stream); | ||
| return runnable; | ||
| } | ||
|
|
||
| /** Runnable may be null. */ | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Branch getRealStream() == null && delayEnded == true is missing coverage. To cover it, a unit test only needs to invoke
endDelay()directly on a pending stream whilegetRealStream() == nulland then callupdateDelay(...):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.
Lets not do this, I didn't realize this change requires changing the class access modifier for PendingStream.