gemini-code-assist[bot] commented on code in PR #37214:
URL: https://github.com/apache/beam/pull/37214#discussion_r2662020734
##########
sdks/typescript/src/apache_beam/worker/state.ts:
##########
@@ -62,20 +90,26 @@ export class CachingStateProvider implements StateProvider {
"base64",
);
if (this.cache.has(cacheKey)) {
+ // Cache hit: move to end (most recently used)
+ this.touchCacheEntry(cacheKey);
return this.cache.get(cacheKey)!;
}
+ // Cache miss: fetch from underlying provider
let result = this.underlying.getState(stateKey, decode);
const this_ = this;
if (result.type === "promise") {
result = {
type: "promise",
promise: result.promise.then((value) => {
+ // When promise resolves, update cache with resolved value
+ this_.evictIfNeeded();
this_.cache.set(cacheKey, { type: "value", value });
return value;
}),
};
Review Comment:
This block has a couple of areas for improvement:
1. **Bug in Eviction Logic (High Severity)**: The call to
`this_.evictIfNeeded()` on line 105 is incorrect. Eviction should only happen
when a *new* entry is added to the cache (which is handled by the call on line
112). When a promise resolves, you are *updating* an existing entry, which
doesn't change the cache size. This call can lead to incorrect cache behavior
by evicting an item when it shouldn't.
2. **Unnecessary `this_` variable (Medium Severity)**: The `const this_ =
this;` on line 99 is not needed. The `.then()` callback is an arrow function,
which lexically captures the `this` context, so you can use `this` directly
inside it.
The suggested code below addresses both points.
```typescript
if (result.type === "promise") {
result = {
type: "promise",
promise: result.promise.then((value) => {
// When promise resolves, update cache with resolved value
this.cache.set(cacheKey, { type: "value", value });
return value;
}),
};
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
