feat(sdks): client sandbox pool implementation#393
feat(sdks): client sandbox pool implementation#393ninan-nn wants to merge 1 commit intoalibaba:mainfrom
Conversation
ninan-nn
force-pushed
the
feature/client_pool_impl
branch
11 times, most recently
from
543d870 to
991e526
Compare
ninan-nn
changed the title
ninan-nn
force-pushed
the
feature/client_pool_impl
branch
from
991e526 to
9ecdc1f
Compare
![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: 9ecdc1f260
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| throw PoolNotRunningException("Cannot acquire when pool state is $state") | ||
| } | ||
| val poolName = config.poolName | ||
| val sandboxId = stateStore.tryTakeIdle(poolName) |
There was a problem hiding this comment.
acquire() only calls tryTakeIdle once, so if that first idle ID is stale/unreachable it immediately falls through to FAIL_FAST/direct-create instead of trying other idle candidates still in the pool. In a queue like [stale, healthy], FAIL_FAST incorrectly throws POOL_EMPTY and DIRECT_CREATE needlessly creates a new sandbox, which breaks expected pool behavior and increases cost/latency.
Useful? React with 👍 / 👎.
| ) | ||
| return | ||
| } | ||
| stateStore.putIdle(poolName, sandboxId) |
There was a problem hiding this comment.
The reconcile loop does not guard stateStore.putIdle, so a transient state-store exception after successful creation exits the tick with already-created sandbox IDs that are neither stored as idle nor cleaned up via onOrphanedCreated. This leaks running sandboxes and also skips failure accounting for that path, so degraded state can be under-reported during store outages.
Useful? React with 👍 / 👎.
| * Configuration for a client-side sandbox pool. | ||
| * | ||
| * @property poolName User-defined name and namespace for this logical pool (required). | ||
| * @property ownerId Unique process identity for primary lock ownership. If not provided, a UUID-based default is generated. |
There was a problem hiding this comment.
Why is it called ownerId? What about using poolId instead?
2 participants
Summary
SandboxPoolwith client-side idle buffer management and configurable acquire policy (FAIL_FAST/DIRECT_CREATE).PoolConfig,PoolCreationSpec,PoolSnapshot,PoolState,PoolStateStore, andInMemoryPoolStateStore.putIdleSandboxPool:resizevsshutdownrace handling (RejectedExecutionExceptionsafe path)drainTimeout)@Deprecatedannotation fromSandboxPoolto align with README guidance while keeping feature marked experimental in docs.Testing
Executed:
cd sdks/sandbox/kotlin && ./gradlew spotlessApply buildcd tests/java && ./gradlew test --tests "*SandboxPool*E2ETest"Result summary:
SandboxPoolSingleNodeE2ETest: 15/15 passedSandboxPoolPseudoDistributedE2ETest: 7/7 passedBreaking Changes
Checklist