Skip to content

evict client only when limit is breached#61

Closed
sarthakaggarwal97 wants to merge 1 commit into
unstablefrom
client-evict-cond
Closed

evict client only when limit is breached#61
sarthakaggarwal97 wants to merge 1 commit into
unstablefrom
client-evict-cond

Conversation

@sarthakaggarwal97

Copy link
Copy Markdown
Owner

No description provided.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
madolson pushed a commit to valkey-io/valkey that referenced this pull request Jan 21, 2026
…table (#3092)

Thanks madolson for pointing out the issue with
#2907.

Whenever we were using the label, it was picking up the head of the
unstable instead of the PR. The reason was that we changed the target to
`pull_request_target`. We had to do that since we wanted to remove the
label after the run is completed.

With this change, it correctly picks up the commit hash of the PR and
checks it out.

To test this, I merged the changes in my forked repository's
[unstable](unstable...sarthakaggarwal97:valkey:unstable),
created a dummy
[PR](sarthakaggarwal97#61), and was able
to verify that the commit of the PR is being checked out in the
[action](https://github.com/sarthakaggarwal97/valkey/actions/runs/21229598167/job/61084986422?pr=61#step:3:81).

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
arshidkv12 pushed a commit to arshidkv12/valkey that referenced this pull request Jan 23, 2026
…table (valkey-io#3092)

Thanks madolson for pointing out the issue with
valkey-io#2907.

Whenever we were using the label, it was picking up the head of the
unstable instead of the PR. The reason was that we changed the target to
`pull_request_target`. We had to do that since we wanted to remove the
label after the run is completed.

With this change, it correctly picks up the commit hash of the PR and
checks it out.

To test this, I merged the changes in my forked repository's
[unstable](valkey-io/valkey@unstable...sarthakaggarwal97:valkey:unstable),
created a dummy
[PR](sarthakaggarwal97#61), and was able
to verify that the commit of the PR is being checked out in the
[action](https://github.com/sarthakaggarwal97/valkey/actions/runs/21229598167/job/61084986422?pr=61#step:3:81).

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Signed-off-by: arshidkv12 <arshidkv12@gmail.com>
@sarthakaggarwal97 sarthakaggarwal97 force-pushed the unstable branch 9 times, most recently from ed90dd4 to fb7b3fb Compare February 13, 2026 19:42
@github-actions

Copy link
Copy Markdown

🤖 AI Review

The PR changes the client eviction condition from >= to > in evictClients(), which is a correct and intuitive fix — clients at exactly the configured memory limit should not be evicted, only those exceeding it. This is consistent with how maxmemory checks work elsewhere in Valkey. The client_eviction_limit == 0 early return guards against the obvious edge case.

Key findings:

  1. 🟡 Missing test coverage — This behavioral change needs a regression test verifying that a client at exactly the eviction limit is NOT evicted. Without it, the fix is untested.
  2. 🟢 Pre-existing size_t addition overflow (not introduced by this PR) — the sum of two size_t values could theoretically overflow, but not realistically on 64-bit.

The code change itself is minimal, focused, and correct. Add a test and it's ready to merge.

@sarthakaggarwal97 sarthakaggarwal97 force-pushed the unstable branch 4 times, most recently from dc95622 to 556180d Compare February 17, 2026 17:03
harrylin98 pushed a commit to harrylin98/valkey_forked that referenced this pull request Feb 19, 2026
…table (valkey-io#3092)

Thanks madolson for pointing out the issue with
valkey-io#2907.

Whenever we were using the label, it was picking up the head of the
unstable instead of the PR. The reason was that we changed the target to
`pull_request_target`. We had to do that since we wanted to remove the
label after the run is completed.

With this change, it correctly picks up the commit hash of the PR and
checks it out.

To test this, I merged the changes in my forked repository's
[unstable](valkey-io/valkey@unstable...sarthakaggarwal97:valkey:unstable),
created a dummy
[PR](sarthakaggarwal97#61), and was able
to verify that the commit of the PR is being checked out in the
[action](https://github.com/sarthakaggarwal97/valkey/actions/runs/21229598167/job/61084986422?pr=61#step:3:81).

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
hpatro pushed a commit to hpatro/valkey that referenced this pull request Mar 5, 2026
…table (valkey-io#3092)

Thanks madolson for pointing out the issue with
valkey-io#2907.

Whenever we were using the label, it was picking up the head of the
unstable instead of the PR. The reason was that we changed the target to
`pull_request_target`. We had to do that since we wanted to remove the
label after the run is completed.

With this change, it correctly picks up the commit hash of the PR and
checks it out.

To test this, I merged the changes in my forked repository's
[unstable](valkey-io/valkey@unstable...sarthakaggarwal97:valkey:unstable),
created a dummy
[PR](sarthakaggarwal97#61), and was able
to verify that the commit of the PR is being checked out in the
[action](https://github.com/sarthakaggarwal97/valkey/actions/runs/21229598167/job/61084986422?pr=61#step:3:81).

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant