chore(affinity): add support for custom affinity key bind and unbind via calloptions#207
Conversation
alkatrivedi
force-pushed
the
feat/metadata-affinity-control
branch
2 times, most recently
from
178db59 to
b656bd2
Compare
alkatrivedi
force-pushed
the
feat/metadata-affinity-control
branch
from
63613f8 to
7b2d24e
Compare
alkatrivedi
force-pushed
the
feat/metadata-affinity-control
branch
2 times, most recently
from
8ef4f50 to
cf6dad8
Compare
|
/gcbrun |
1 similar comment
|
/gcbrun |
alkatrivedi
force-pushed
the
feat/metadata-affinity-control
branch
2 times, most recently
from
2e18593 to
f88f6c3
Compare
alkatrivedi
changed the title
|
/gcbrun |
alkatrivedi
force-pushed
the
feat/metadata-affinity-control
branch
4 times, most recently
from
e239b3e to
2fc8798
Compare
alkatrivedi
force-pushed
the
feat/metadata-affinity-control
branch
4 times, most recently
from
eef6e10 to
a2e0fea
Compare
alkatrivedi
force-pushed
the
feat/metadata-affinity-control
branch
from
a2e0fea to
de4d036
Compare
|
/gemini review |
|
/gcbrun |
| this.addChannel(); | ||
| } | ||
|
|
||
| this.cleanupTimer = setInterval(() => this.cleanupIdleKeys(), 30000); |
| this.addChannel(); | ||
| } | ||
|
|
||
| this.cleanupTimer = setInterval(() => this.cleanupIdleKeys(), 30000); |
There was a problem hiding this comment.
move these interval configurations to const
const CLEANUP_INTERVAL_MS = 30000; // Sweep every 30 seconds
const IDLE_TIMEOUT_MS = 180000; // Keys are considered idle if unused for more than 3 minutes
| existingChannelRef.affinityCountIncr(); | ||
| } | ||
|
|
||
| isBound(affinityKey: string): boolean { |
| next: Function | ||
| ) => { | ||
| // Always decrement stream count on status receive (stream closed) | ||
| channelRef.activeStreamsCountDecr(); |
There was a problem hiding this comment.
What if the status is retryable like "Unavailable" ? When we retry the stream with resume token , the request should go on the same channel or different ?
This should have been applicable for previous implementation as well when session was used as affinity key ?
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| argument?: any | ||
| argument?: any, | ||
| overrideAffinityKey?: string |
There was a problem hiding this comment.
Update the method definition with this new parameter.
| channelFactory.unbind(boundKey); | ||
| } | ||
| } | ||
| channelRef.activeStreamsCountDecr(); |
There was a problem hiding this comment.
Why did we remove this ? Is it because we decrementing the count when we receive the status ?
There was a problem hiding this comment.
Is postProcess not called for all scenarios ?
| const callback = callProperties.callback; | ||
|
|
||
| const preProcessResult = preProcess(channelFactory, path, argument); | ||
| const affinityConfig = channelFactory.getAffinityConfig(path); |
There was a problem hiding this comment.
We already call channelFactory.getAffinityConfig(path) in preProcess method, could we have moved fetching "affinityKeyFromCallOptions" in "preProcess" method to avoid multiple calls to this ?
| "@grpc/proto-loader": "^0.7.0", | ||
| "@google-cloud/spanner": "^6.0.0", | ||
| "@grpc/proto-loader": "^0.7.0", | ||
| "@types/glob": "8.1.0", |
There was a problem hiding this comment.
Why do we need these extra imports ?
3 participants
This PR contains changes for handling support for custom affinity key changes
The binding and unbinding logic for a custom affinity key