feat: momento leaderboard client #438

brayniac · 2025-01-25T16:49:12Z

Adds client functionality for the Momento Leaderboard API.

The control client isn't necessary/used in the leaderboard client.

The leaderboard client, unlike the cache client, uses a reosurce oriented pattern. The user calls `leaderboard` on the leaderboard client to get a leaderboard object for a specific cache- and leaderboard-name. They then invoke specific operations on that leaderboard without needing to pass in the cache- and leaderboard-name.

Refactors the leaderboard to use the method, request, and response names as per the JS client.

Moves the `Leaderboard` struct and impl into a dedicated mod.

Adds comprehensive integration tests for the leaderboard client. Applies fixes and refactoring to get tests green.

Ensure the score range doesn't have a nonsensical range, eg (+inf, -inf).

kvcache

I ran out of time today about halfway through. I'll give this feedback before continuing to review tomorrow.

src/leaderboard/config/configuration.rs

kvcache · 2025-02-05T01:13:30Z

src/leaderboard/leaderboard_client.rs

+/// Client to work with Momento Leaderboards.
+#[derive(Clone, Debug)]
+pub struct LeaderboardClient {
+    data_clients: Vec<SLbClient<InterceptedService<Channel, HeaderInterceptor>>>,


Momento typically does not use import aliases, and tends to avoid obfuscating abbreviation. Is there a compelling reason to write SLbClient rather than matching the de-facto code style and using the struct's name LeaderboardClient? Alternatively, what does S mean? Can you write a comment explaining this name, or switch to a clearer name?

If you're struggling with the protos squatting on the name, consider aliasing to something clearer like as ProtoLeaderboardClient, or use momento_protos::leaderboard::leaderboard_client as leaderboard_proto; and refer to leaderboard_proto::LeaderboardClient.

(here and everywhere "SLbClient" is defined)

I've changed this to qualify from the leaderboard proto mod instead as per your suggestion in 5803c70

src/leaderboard/leaderboard_resource.rs

kvcache · 2025-02-05T01:36:25Z

src/leaderboard/messages/data/fetch_by_rank.rs

+    end_exclusive: u32,
+}
+
+impl From<Range<u32>> for RankRange {


should this be implemented for RangeInclusive as well?

Done in d749551. I'm unclear how to convey the unlikely but troubling edge case if a user provides u32::MAX as the end index inclusive and we convert to exclusive. I've just clamped to u32::MAX in my implementation. Any suggestions?

src/leaderboard/leaderboard_resource.rs

kvcache · 2025-02-05T02:28:37Z

src/leaderboard/messages/data/fetch.rs

+    /// Returns the ranked elements in the response.
+    pub fn elements(&self) -> &[RankedElement] {
+        &self.elements
+    }


there should be an into_elements(self) as well here to avoid forcing users to clone if they want owned elements.

Added in e61b87e and d6722e9.

For my benefit, is one or the other preferred/expected/more commonplace? Or should an API have both available? It seems that this depends how many times the caller wants to access the underlying data.

src/leaderboard/messages/data/fetch_by_rank.rs

kvcache · 2025-02-05T02:32:26Z

src/leaderboard/messages/data/fetch_by_rank.rs

+            response
+                .elements
+                .iter()
+                .map(|v| RankedElement {


getrank, fetchbyscore, fetchbyrank - is v the same type in all 3? Just impl From<Whatever V Is> for RankedElement and replace this line and the other 2 with .map(|v| v.into()) instead of repeating the same marshaling stanza

Nice. Fixed in 305d247.

kvcache · 2025-02-05T02:35:09Z

src/leaderboard/messages/data/fetch_by_score.rs

+    }
+
+    /// Validates the score range.
+    pub fn validate(&self) -> MomentoResult<()> {


why is this public? Why is it possible to create an invalid ScoreRange? Shouldn't new() return a MomentoResult<Self>?

Should new just .clamp min and max to f64::MIN and f64::MAX instead of making it an error to write 1.0..?

1.0.. not working was an oversight my on part. I've added From<RangeFrom<f64>> and From<RangeTo<f64>> to handle those cases in 0576f7a

The error cases are when the user passes in min=fp64::POS_INFINITY or max=fp64::NEG_INFINITY. We translate those to the proto Unbounded message. That's why the check val.is_finite() isn't enough before marshalling to Unbounded: if min or max is the wrong infinity, then it's an error.

I can have new return MomentoResult<Self> 👍

Well maybe not. The top level methods take as input impl Into<ScoreRange>. If we let new return MomentoResult<ScoreRange> then the callers will have to invoke the function with something like client.fetch_by_score((1.0..5.0).into()?), right? Because the From<Range<f64>> implementation is for ScoreRange, not MomentoResult<ScoreRange>. See j1c7c02d

kvcache · 2025-02-05T02:40:43Z

src/leaderboard/messages/data/fetch_by_score.rs

+impl From<Option<ScoreRange>> for ScoreRange {
+    fn from(val: Option<ScoreRange>) -> Self {
+        val.unwrap_or_else(ScoreRange::unbounded)
+    }
+}


This appears to be unused, and it is unclear to me that unbounded means the same thing as None for a whole score range. If you want to keep it, what's the utility here, and how should this be communicated so it doesn't surprise people that None means Everything?

Good catch -- this was a leftover while I was iterating on the UI. I've removed this in e930128

Previously we used an alias to the leaderboard proto client. We instead qualify from the proto mod for clarity.

In the configuration we communicated the client timeout as a "deadline". We refactor to use the name "client_timeout" throughout the leaderboard code. More broadly (cache/topics) we will have to rename later.

Since we model the leaderboard resource as an object, we align with rust naming conventions to use `len` to measure its size.

Adds documentation about the default ordering for fetch by score, fetch by rank, and get rank.

Renames `MomentoRequest` to `LeaderboardRequest` for clarity. This way we don't have `MomentoRequest` defined in various places (here and in the cache client).

Removes helper function and inlines since it's only used in one place.

Implements `From<>` on the proto `RankedElement` for the SDK `RankedElement` to simplify marshalling code.

In order to make the various `From` implementations continue to work, we defer validation until the RPC call.

Prefer `impl IntoIterator<Item = impl Into<Element>>` for eleemnts.

brayniac added 4 commits January 25, 2025 08:47

feat: momento leaderboard client

11f7be4

Adds client functionality for the Momento Leaderboard API.

Merge branch 'main' into leaderboard

116a5bb

cleanup

3ad3ac6

docs fixes

68a19c7

brayniac force-pushed the leaderboard branch from 0b27977 to 68a19c7 Compare January 25, 2025 17:41

malandis added 9 commits January 31, 2025 15:07

refactor: remove control client

a77d839

The control client isn't necessary/used in the leaderboard client.

refactor: use standard naming

a43f39d

Refactors the leaderboard to use the method, request, and response names as per the JS client.

refactor: move Leaderboard to separate module

91120d0

Moves the `Leaderboard` struct and impl into a dedicated mod.

test: test leaderboard client

3825134

Adds comprehensive integration tests for the leaderboard client. Applies fixes and refactoring to get tests green.

chore: simplify score range for unbounded case

50e6e64

chore: validate score range

b503cba

Ensure the score range doesn't have a nonsensical range, eg (+inf, -inf).

fix: clippy

3694822

chore: remove stale comments

6b42e52

malandis requested a review from a team February 5, 2025 00:48

kvcache reviewed Feb 5, 2025

View reviewed changes

malandis added 14 commits February 5, 2025 09:30

chore: replace SLbClient type alias with mod alias

5803c70

Previously we used an alias to the leaderboard proto client. We instead qualify from the proto mod for clarity.

refactor: rename "deadline" to "client_timeout"

ada9c97

In the configuration we communicated the client timeout as a "deadline". We refactor to use the name "client_timeout" throughout the leaderboard code. More broadly (cache/topics) we will have to rename later.

refactor: rename Leaderboard::length to Leaderboard::len

4f32b66

Since we model the leaderboard resource as an object, we align with rust naming conventions to use `len` to measure its size.

docs: clarify default ordering and meaning

c5800e6

Adds documentation about the default ordering for fetch by score, fetch by rank, and get rank.

refactor: return &str from accessors

f31495c

docs: clarify MomentoRequest documentation

2e155bd

refactor: MomentoRequest -> LeaderboardRequest

87dc81a

Renames `MomentoRequest` to `LeaderboardRequest` for clarity. This way we don't have `MomentoRequest` defined in various places (here and in the cache client).

chore: remove map_and_collect helper function

b2a03a9

Removes helper function and inlines since it's only used in one place.

chore: remove unused ScoreRange converter

e930128

chore: add into_elements on FetchResponse

e61b87e

chore: add into_elements on GetRankResponse

d6722e9

chore: simplify RankedElement conversation from proto with From

305d247

Implements `From<>` on the proto `RankedElement` for the SDK `RankedElement` to simplify marshalling code.

fix: clippy

cf2604b

chore: add From<RangeInclusive<>> impl for RankRange

d749551

malandis added 5 commits February 5, 2025 14:40

chore: impl From<RangeFrom<>>, From<RangeTo<>> for ScoreRange

0576f7a

chore: more range converters

d9dbe3f

refactor: make score range validator pub crate

1c7c02d

In order to make the various `From` implementations continue to work, we defer validation until the RPC call.

refactor: remove IntoIds in favor of more general IntoIterator

67da8e0

refactor: use more general type for upsert argument

981b5b7

Prefer `impl IntoIterator<Item = impl Into<Element>>` for eleemnts.

malandis requested a review from kvcache February 6, 2025 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: momento leaderboard client #438

feat: momento leaderboard client #438

brayniac commented Jan 25, 2025

kvcache left a comment

kvcache Feb 5, 2025

malandis Feb 5, 2025

kvcache Feb 5, 2025

malandis Feb 5, 2025 •

edited

Loading

kvcache Feb 5, 2025

malandis Feb 5, 2025 •

edited

Loading

kvcache Feb 5, 2025

malandis Feb 5, 2025

kvcache Feb 5, 2025

malandis Feb 5, 2025

malandis Feb 5, 2025 •

edited

Loading

malandis Feb 5, 2025

kvcache Feb 5, 2025

malandis Feb 5, 2025

feat: momento leaderboard client #438

Are you sure you want to change the base?

feat: momento leaderboard client #438

Conversation

brayniac commented Jan 25, 2025

kvcache left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malandis Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malandis Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malandis Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malandis Feb 5, 2025 •

edited

Loading

malandis Feb 5, 2025 •

edited

Loading

malandis Feb 5, 2025 •

edited

Loading