Conversation
69fa9ff to
595a94c
Compare
Test Results 7 files + 2 7 suites +2 2m 47s ⏱️ -48s Results for commit a4c8893. ± Comparison against base commit 6d70da2. This pull request removes 25 and adds 45 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
350a09e to
6eb0d7c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47324891fd
ℹ️ 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".
| }); | ||
| // todo: This is temporary until we wrap the returned permit into an InvokePermit | ||
| // that invoker permit will carry the inner permit as opaque type. | ||
| let (permit, memory_lease) = run_permit.take_invoker_permit(); |
There was a problem hiding this comment.
Keep user limiter permits alive for invocation lifetime
This extracts only the invoker semaphore/memory lease and then drops run_permit at the end of the match arm, which triggers ReservedResources::drop and sends PermitReleased for user resources immediately. As a result, LimitKeyConcurrency counters are decremented right after dispatch, so long-running invocations no longer consume user-concurrency slots and configured user limits are effectively bypassed under concurrent load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Coming soon...
c203f6f to
454db8a
Compare
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks a lot for adding support for the user limiter @AhmedSoliman. The changes look great. +1 for merging :-)
As a follow-up, we need to figure out how to gate this feature.
| // Copyright (c) 2023 - 2026 Restate Software, Inc., Restate GmbH. | ||
| // All rights reserved. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the LICENSE file. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0. |
There was a problem hiding this comment.
As you've already raised the question, we need to find a way how to make this feature (the hierarchical limits) only accessible for enterprise users. Some variants that claude has seen in the wild are:
- Cockroachdb: They put everything under their Cockroackdb Software License which allows free usage as long as your revenue is below $10 million. They enforce it by requiring a license key and users of the free tier can ask for a free tier license key. Before, cockroachdb was putting enterprise features under a different license but they moved away from it in favor of the CSL.
- Redpanda: They put the enterprise features under a different license (redpanda community license) and require a license key for continued usage. If users don't have a valid license key, then the feature will degrade gracefully after a period of 30 days. In our case, this could mean that we no longer accept the addition/modification of hierarchical limits.
- Gitlab: They have dedicated enterprise features as extensions to their open source features. All enterprise features live under a separate directory
eeand they build a community edition and enterprise edition. For the latter, they replace the community edition features with their ee implementations. They also check license keys if ee features are used at runtime.
We don't have to answer this question right now but until we release, we probably need to find an answer.
One theme that seems to be very common is to prevent enterprise feature usage by having a license key.
Personally, I like the Redpanda model which allows users to try enterprise features out (w/o a high entrance barrier) and if they like them, then they have an incentive to get a paid subscription. But that's ultimately more of a product decision.
There was a problem hiding this comment.
I'll leave things as is, let's discuss internally.
Stack created with Sapling. Best reviewed with ReviewStack.