Support proper async propagation for GAX retrying futures#11629
Conversation
amarziali
added
type: feature
This comment has been minimized.
This comment has been minimized.
amarziali
force-pushed
the
andrea.marziali/gax
branch
from
ebd0563 to
87e342a
Compare
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
amarziali
requested review from
dougqh,
jordan-wong and
mcculls
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87e342a9e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Thanks for the follow up changes
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
2 participants
What Does This Do
This PR is a result of investigating why the PubSub instrumentation was leaking continuation.
It appeared that we have futures that are retriable and in fact the guava-concurrent was capturing continuations on those futures as soon as a listener was set.
Now, for retriable futures, gax retry calls setAttemptFuture twice per attempt:
The guava AbstractFuture.addListener instrumentation captures a trace continuation on each attempt-completion listener. The placeholder's listener never runs and its continuation is never resolved
In this PR we introduce a new module to deal with those gax specific futures. Since there are no signals to hook on to know if the first future will be run or not, we just simply cancel the previously taken continuation when the second is captured.
In any case the first continuation won't be run when the second fire so the fix seems safe to me.
That approach also solves continuation leakage issues in the pubsub instrumentation so that the strictTraceWrites hack is now removed
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]