Closed xaionaro closed 1 year ago
Patch coverage: 66.07
% and project coverage change: -1.58
:warning:
Comparison is base (
57569d4
) 63.37% compared to head (ab3087b
) 61.79%.:exclamation: Current head ab3087b differs from pull request most recent head c13b022. Consider uploading reports for the commit c13b022 to get more accurate results
:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
this is gonna take a while to review, but i know what it is about
I've switched the base from main
to develop
, since the PR contains API-breaking changes.
since this touches state resume in a lot of places, id like to see a test with that. maybe a step with sleep 30, sigint the server and resume
that would also show if there's any changes in the log output
I've started to redesign signaling
: there was a known and shameful problem I had no time to fix in xcontext
, and it correlates with some of the feedback. So I guess I finally need to find time to redesign the thing and fix that :)
Will publish today-tomorrow.
UPDATE: Done. But now integration tests are failing. Looking into it...
since this touches state resume in a lot of places, id like to see a test with that. maybe a step with sleep 30, sigint the server and resume
I thought we have integration tests, which check this. No?
Commit "[pkg/signaling] Add a package to handle Pause signal" was rewritten from scratch and requires a full review. In other commits the changes are isolated to minimalistically address the provided feedback.
very nice job in refactoring the signaling part
I thought we have integration tests, which check this. No?
yeah, that's fair. we'll also see if it breaks on internal usage
Pushed a new version of the PR. Overall:
IsSignaledWith
signature.develop
and adapted to new IsSignaledWith
function signature.thanks for addressing all of the review items; merging
What
Here I do substitution of
xcontext
togo-belt
andsignallers
. The APIs are mostly compatible, so this was a pretty mechanistic stupid job (could mostly be done withsed
).Caveats:
xcontext
binds together three different things: observability API, context and signals (currently used to propagatePause
signal across the application). And it is problematic to make the migration of observability API and signals separately, because if I migrate one thing, it breaks another thing. So I migrate them together in one commit.xcontext
bothCancel
andPause
as processed by the same handler. But after migrationCancel
is processed by standardcontext
API, whilePause
is processed by additional signal-handler. Because of that it is now no unified API between these two.Why
TLDR:
xcontext
is succeeded bygo-belt
.xcontext
existed for some time, there was gathered the feedback and in resultgo-belt
is a better version of similar ideas.More details
The previous design had few very serious problems:
Caveats
above) and is not scalable horizontally (the amount of observability tools are limited).context.Context
, which causes different inconveniences with necessity to type-cast and do other similar stuff. Also the custom context type started to propagate to public APIs which never was the intention (public APIs should always use only the standard context).xcontext
is on life-support, whilego-belt
is actually supported.There were also other known (less serious) problems, and in
go-belt
all that is solved.How
The PR consists of 4 commits:
Pause
(though it can propagate other signals as well). This package mostly just replicates the related code ofxcontext
.xcontext
togo-belt
andsignaller
accordingly. This is mostly mechanistic change, which stupidly changes the code to an equivalent. Sorry for a huge commit, but it is difficult to split without breaking functionality :(xcontext
is deleted (packageperf
is kept and moved out ofxcontext
).