feat(tdigest): add support of TDIGEST.RESET command by SharonIV0x86 · Pull Request #2826 · apache/kvrocks

SharonIV0x86 · 2025-03-13T17:57:07Z

This closes #2804

This PR ~~is a Work In Progress and~~ adds the support for the TDIGEST.RESET command https://redis.io/docs/latest/commands/tdigest.reset/.

I’m currently working on implementing the TDIGEST.RESET command. However, I noticed that there doesn’t seem to be a built-in way to delete centroids from a TDigest. And in order to reset a digest we need to delete the centroids that its containing, correct me if im wrong.
Also, to do this, one way i see is that we can manually delete them by iterating over the stored centroids and removing them one by one.

Additionally there seems to be already a Reset() function defined.

kvrocks/src/types/tdigest.cc

Line 117 in b8db0e7

void Reset() {

And i am really confused about the two implementation of the TDigest in kvrocks, one in types/redis_tdigest.cc and one in type/tdigest.cc.
The existing TDigest commands uses the TDigest implementation from types/redis_tdigest.cc and it currently has no Reset() function defined.

So any assistance or reviews on this PR are highly appreciated as i am new to the codebase.

To the best of my knowledge DummyCentroids class is being used as a class to iterate over centroids in Quantile function, maybe if its possible we can add some functionality in this class to delete a centroid.

SharonIV0x86 · 2025-03-14T06:00:55Z

@PragmaTwice I updated the Reset function so that it resets the metadata fields and writes the new metadata back.

From what I understand, this should only clear the high-level info, removing references to the centroids without actually deleting them. The centroids should still exist until compaction eventually cleans them up.

But when I run TDIGEST.RESET, it gets stuck in an infinite loop. I’m not sure why any ideas on what might be causing this?

PragmaTwice · 2025-03-15T03:12:13Z

@LindaSummer Hi, could you help to review this PR when you have time?

LindaSummer · 2025-03-15T03:15:11Z

@LindaSummer Hi, could you help to review this PR when you have time?

Hi @PragmaTwice ,

Of course!
I will take a review today.😊

Best Regards,
Edward

LindaSummer · 2025-03-15T03:32:26Z

@PragmaTwice I updated the Reset function so that it resets the metadata fields and writes the new metadata back.

From what I understand, this should only clear the high-level info, removing references to the centroids without actually deleting them. The centroids should still exist until compaction eventually cleans them up.

But when I run TDIGEST.RESET, it gets stuck in an infinite loop. I’m not sure why any ideas on what might be causing this?

Hi @SharonIV0x86 ,

Thanks very much for your effort!😊

If I understand correctly, the compaction is handled by rocksdb and we should delete related keys to free the reference of related keys.

In my subkey design, I use a flag prefix for centroid keys. It could be deleted by some method like deletebyprefix.

I'm now without a computer by my side, I would provide the function link of prefix deletion today after back home.😊

For the infinite loop issue, I will try to find the cause if you don't mind.😊

Best Regards,
Edward

SharonIV0x86 · 2025-03-15T04:48:59Z

@LindaSummer The looping issue is fixed and it works as expected.

SharonIV0x86 · 2025-03-15T06:54:00Z

@LindaSummer If possible also provide your implementation too, as you mentioned it can be used by some method like deletebyprefix but ig its not added to kvrocks yet. So ill compare yours and mine and whichever is better we'll go with it.

LindaSummer

Hi @SharonIV0x86 ,

Thanks very much for your contribution! 😊

Left with some comments.

By the way, could you add some go integration test for this command to cover the new code in this PR if possible?

Best Regards,
Edward

src/types/redis_tdigest.cc

LindaSummer

Hi @SharonIV0x86 ,

Reviewed with some comments. 😊

Best Regards,
Edward

src/types/redis_tdigest.cc

src/commands/cmd_tdigest.cc

Added go integration tests for TDIGEST.RESET Made requested changes. Added go test case

SharonIV0x86 · 2025-03-17T13:16:42Z

@LindaSummer I cross-checked with the redis-stack and it returns OK for both the cases.

If digest is empty and you execute a reset: returns OK.
If digest is non-empty and you execute a reset: return OK.

Now this aligns with the functionality of redis-stack. Also i added a go test case for this. Please review when you have time 😄

LindaSummer

Hi @SharonIV0x86 ,

Thanks for your effort.😊
Generally LGTM.

Leave with some nitpick comments.

Best Regards,
Edward

src/types/redis_tdigest.cc

tests/gocase/unit/type/tdigest/tdigest_test.go

SharonIV0x86 · 2025-03-18T14:43:00Z

@LindaSummer Made the changes, pls check now and lmk so i can proceed with opening a PR for modifying docs.

LindaSummer

Hi @SharonIV0x86 ,

Thanks for your contribution!

Left one comment.

Other parts are LGTM.

Best Regards,
Edward

src/types/redis_tdigest.cc

fix(cluster): resolve forbidden slot range cleanup bug during the slot migration (apache#2829) Made smoll fixes. Made smoll changes

LindaSummer · 2025-03-19T01:05:30Z

src/cluster/cluster.cc

        }
      };  // Stop forbidding writing slot to accept write commands
-      if (slot_range == srv_->slot_migrator->GetForbiddenSlotRange()) srv_->slot_migrator->ReleaseForbiddenSlotRange();
+      if (slot_range.HasOverlap(srv_->slot_migrator->GetForbiddenSlotRange())) {


Hi @SharonIV0x86 ,

It seems that this file change should not be shown in this PR.

Could you take a review on your commit?

Best Regards,
Edward

This seems like a mistake, maybe merging the base branch with this can fix it?

LindaSummer · 2025-03-19T07:56:27Z

src/commands/cmd_tdigest.cc

 constexpr auto kInfoObservations = "Observations";
 constexpr auto kInfoTotalCompressions = "Total compressions";
 }  // namespace
-


Hi @SharonIV0x86 ,

Could you help keep this blank line for namespace section if you don't mind? 😊

Best Regards,
Edward

LindaSummer · 2025-03-19T09:51:23Z

Hi @SharonIV0x86

LGTM

Thanks very much for your contribution!😊

Best Regards,
Edward

Kept a blank line for namespace.

SharonIV0x86 · 2025-03-19T09:58:44Z

Hi @SharonIV0x86

LGTM

Thanks very much for your contribution!😊

Best Regards, Edward

Done.
Also kindly take a look at this, as i want to work on this issue next.

sonarqubecloud · 2025-03-20T04:47:44Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
61.7% Coverage on New Code
13.8% Duplication on New Code

See analysis details on SonarQube Cloud

WIP: tdigest.reset

02cc890

PragmaTwice changed the title ~~feat(tdigest): Added TDIGEST.RESET command. WIP~~ feat(tdigest): add suuport of TDIGEST.RESET command Mar 14, 2025

PragmaTwice changed the title ~~feat(tdigest): add suuport of TDIGEST.RESET command~~ feat(tdigest): add support of TDIGEST.RESET command Mar 14, 2025

SharonIV0x86 added 2 commits March 14, 2025 11:19

Fixed lint

c845eb5

Fixed lint again

168de64

SharonIV0x86 added 2 commits March 14, 2025 13:47

Fixed missing output of redis::RESP_OK

3c5331b

removed unnecessary comment

82d68eb

SharonIV0x86 marked this pull request as ready for review March 14, 2025 08:29

LindaSummer reviewed Mar 15, 2025

View reviewed changes

src/types/redis_tdigest.cc Show resolved Hide resolved

src/types/redis_tdigest.cc Outdated Show resolved Hide resolved

Removed comments, added logic for removing centroid keys by prefix

fa22727

SharonIV0x86 requested a review from LindaSummer March 15, 2025 18:28

LindaSummer reviewed Mar 16, 2025

View reviewed changes

src/types/redis_tdigest.cc Outdated Show resolved Hide resolved

src/commands/cmd_tdigest.cc Show resolved Hide resolved

Merged similar commits together to keep history clean.

abc04c7

Added go integration tests for TDIGEST.RESET Made requested changes. Added go test case

SharonIV0x86 force-pushed the feat/tdigest.reset branch from b2e04c0 to abc04c7 Compare March 17, 2025 13:15

LindaSummer reviewed Mar 18, 2025

View reviewed changes

src/types/redis_tdigest.cc Outdated Show resolved Hide resolved

Squashed similar commits

79718dd

fix(cluster): resolve forbidden slot range cleanup bug during the slot migration (apache#2829) Made smoll fixes. Made smoll changes

SharonIV0x86 requested a review from LindaSummer March 18, 2025 15:14

SharonIV0x86 force-pushed the feat/tdigest.reset branch from 3cc1cff to 79718dd Compare March 18, 2025 15:16

SharonIV0x86 mentioned this pull request Mar 18, 2025

Add docs for TDIGEST.RESET apache/kvrocks-website#285

Merged

LindaSummer reviewed Mar 19, 2025

View reviewed changes

Merge branch 'unstable' into feat/tdigest.reset

f45a9ab

LindaSummer reviewed Mar 19, 2025

View reviewed changes

Update cmd_tdigest.cc

22ad83a

Kept a blank line for namespace.

Merge branch 'unstable' into feat/tdigest.reset

1f38132

PragmaTwice approved these changes Mar 20, 2025

View reviewed changes

PragmaTwice merged commit f025147 into apache:unstable Mar 20, 2025
36 checks passed

SharonIV0x86 deleted the feat/tdigest.reset branch March 23, 2025 07:13

Conversation

SharonIV0x86 commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SharonIV0x86 commented Mar 14, 2025

Uh oh!

PragmaTwice commented Mar 15, 2025

Uh oh!

LindaSummer commented Mar 15, 2025

Uh oh!

LindaSummer commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SharonIV0x86 commented Mar 15, 2025

Uh oh!

SharonIV0x86 commented Mar 15, 2025

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SharonIV0x86 commented Mar 17, 2025

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SharonIV0x86 commented Mar 18, 2025

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LindaSummer Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SharonIV0x86 Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

LindaSummer Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LindaSummer commented Mar 19, 2025

Uh oh!

SharonIV0x86 commented Mar 19, 2025

Uh oh!

sonarqubecloud bot commented Mar 20, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SharonIV0x86 commented Mar 13, 2025 •

edited

Loading

LindaSummer commented Mar 15, 2025 •

edited

Loading

LindaSummer Mar 19, 2025 •

edited

Loading

LindaSummer Mar 19, 2025 •

edited

Loading