Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLUSTER SLOT-STATS ORDERBY when stats are the same, compare by slot in ascending order #710

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

enjoy-binbin
Copy link
Member

Test failed in my local:

*** [err]: CLUSTER SLOT-STATS ORDERBY LIMIT correct response pagination, where limit is less than number of assigned slots in tests/unit/cluster/slot-stats.tcl
Expected [dict exists 0 0 1 0 2 0 3 0 4 0 16383] (context: type source line 64 file /xxx/tests/unit/cluster/slot-stats.tcl cmd {assert {[dict exists $expected_slots $slot]}} proc ::assert_slot_visibility level 1)

It seems that when the stat is equal, that is, when the key-count is equal,
the qsort performance will be different. When the stat is equal, we compare
by slot (in ascending order).

…n ascending order

Test failed in my local:
```
*** [err]: CLUSTER SLOT-STATS ORDERBY LIMIT correct response pagination, where limit is less than number of assigned slots in tests/unit/cluster/slot-stats.tcl
Expected [dict exists 0 0 1 0 2 0 3 0 4 0 16383] (context: type source line 64 file /xxx/tests/unit/cluster/slot-stats.tcl cmd {assert {[dict exists $expected_slots $slot]}} proc ::assert_slot_visibility level 1)
```

It seems that when the stat is equal, that is, when the key-count is equal,
the qsort performance will be different. When the stat is equal, we compare
by slot (in ascending order).

Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin requested a review from madolson June 28, 2024 05:34
Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah qsort is not stable. Nice fix @enjoy-binbin!

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.21%. Comparing base (1269532) to head (e757c39).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #710      +/-   ##
============================================
- Coverage     70.23%   70.21%   -0.03%     
============================================
  Files           111      111              
  Lines         60203    60207       +4     
============================================
- Hits          42286    42274      -12     
- Misses        17917    17933      +16     
Files Coverage Δ
src/cluster_slot_stats.c 87.87% <100.00%> (+0.51%) ⬆️

... and 12 files with indirect coverage changes

@madolson madolson merged commit 2979fe6 into valkey-io:unstable Jun 28, 2024
19 checks passed
@enjoy-binbin enjoy-binbin deleted the cluster_slot_stats_order branch June 29, 2024 00:18
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.

3 participants