🏞️ Implement streams commands #86

Terkwood · 2020-06-07T19:28:45Z

Add stream commands

This change set resolves #36. 🚢

API design 🎨

We gladly welcome feedback on the design. You can read the method defs in command.ts to see the most up-to-date version of the command inputs & outputs

Strongly Typed Message IDs

We implemented strong types for stream entry IDs:

export interface XId {
  unixMs: number;
  seqNo: number;
}

We made an effort to ensure flexibility in the command interfaces. For instance, in xadd, you may also pass simple numbers, or paired numbers [1000,10], or the special string "*".

Methods which return XIds will always return the exact form, so that you can deconstruct them with minimal pain.

See XId and the related types in streams.ts for details.

Return type for XREAD 📕

We offer an interface for the XREAD reply type which is hopefully less annoying to use than the Redis default array type. Please note that we return field_value pairs as a Map.

export interface XMessage {
  id: XId;
  field_values: Map<string, string>;
}
export type XReadStream = { key: string; messages: XMessage[] };
export type XReadReply = XReadStream[];

implement a nice return type for XREAD and XREADGROUP

Allow input of record objects in XADD

In xadd, we chose to allow input of arbitrary Record<string | number,string | number> objects, as well as Map<string | number,string | number>.

⚠ This allows greater flexibility for input, but may end up being a little confusing since the field_value outputs of xread and xreadgroup are always Map<string,string>. ⚠

support record objects in XADD
Remove support for array of string field_values in XADD, touch up docs to reflect
expose a single xadd implementation & interface that allows default options

Commands to implement and test

docs and sanity checks

We made an effort to verify that argument names match the canonical names exposed by Redis. One exception is references to ID in Redis docs, which we call xid: XId in order to match the parsed, typed flavor of data. Minimal command form should be included in each doc string.

The JsDoc approach isn't very nice since only typescript export statements can trigger deno doc, but at least these docs are available to developers via their editors.

special reply types to model

Some hints for reply types https://github.com/Terkwood/redis-rs/blob/0d126f54d6970b931594bf677b09a368af70617d/src/streams.rs#L231

pending: use a discriminated union to represent the various response formats
info: similar to above, and be careful to heed the disclaimers in the Redis docs

Additional requirements

Update README with example
Test suite record cleanup
add test demonstrating multiple consumers handling unique messages (fine grained work splitting) Do you or are you planning to support redis.client.xgroup? #79 (comment) and Do you or are you planning to support redis.client.xgroup? #79 (comment)
address every TODO
match doc params to names (xid vs id, etc)
check that isNumber areas are sane

Always Remember

pass deno lint (see chore: pass "deno lint" #98 for inspiration). The CI will enforce this anyway.
pass deno fmt --check. The CI will enforce this also.

uki00a · 2020-06-30T19:11:52Z

Thanks @Terkwood! I'll review this PR in the coming days.

command.ts

uki00a · 2020-06-30T23:09:47Z

tests/stream_test.ts

+
+  let created = await client.xgroup_create(key, groupName, "$", true);
+  assertEquals(created, "OK");
+  try {


I think it's better to use assertThrowsAsync.

Suggested change

try {

await assertThrowsAsync(async () => {

await client.xgroup_create(

key,

groupName,

0,

true

);

}, Error, "-BUSYERR");

Altered, but for now the Error type and exact error message are left undefined. The message text contains a bunch of whitespace and a newline and is a little tricky to match. Haven't tracked down the Error class yet. I can come back to this if it's a sticking point...

AssertionError: Expected error to be instance of "Error", but got "Error".

Aha, got the message part matching, at least!

tests/stream_test.ts

uki00a · 2020-07-01T00:40:05Z

command.ts

@@ -286,6 +313,352 @@ export type RedisCommands = {
  srem(key: string, ...members: string[]): Promise<Integer>;
  sunion(...keys: string[]): Promise<BulkString[]>;
  sunionstore(destination: string, ...keys: string[]): Promise<Integer>;
+  // Stream
+  /**


nice documents!

uki00a · 2020-07-01T00:46:58Z

stream.ts

+
+export interface XMessage {
+  xid: XId;
+  field_values: Map<string, string>;


Hmmm, it's hard to decide whether Map or Record is better... 🤔

uki00a · 2020-07-01T00:52:54Z

Looks good to me overall, but a few things need to be fixed.
Please check it.

Terkwood · 2020-07-01T02:53:01Z

Thanks for the fast review! I will be able to respond on Thursday or Friday! The comments look straightforward.

I don't have a strong opinion on Record vs Map for the return type from the xread and xreadgroup methods, but my ultimate rationale was that iterating thru Maps seems a bit less clunky: https://stackoverflow.com/questions/16174182/typescript-looping-through-a-dictionary

But for accessing individual fields, Record (dictionary a.b) syntax seems a little friendlier to the library user.

I'm happy to use either choice.

Co-authored-by: uki00a <uki00a@gmail.com>

...but leaves the error type and message undefined.

uki00a

Thanks for submitting the changes!
There is just one thing to fix.
Please check it.

tests/stream_test.ts

uki00a · 2020-07-02T20:01:47Z

@Terkwood
IMO it's not a bad idea to adopt Map over Record (because Map keeps the order of the fields in the stream entry).
Having said that, I'd like to hear the opinions of other people, so I'll open the issue about it.

Let's merge this PR for now!

uki00a · 2020-07-02T20:04:49Z

Merged, thanks a lot @Terkwood!

Terkwood · 2020-07-02T20:09:45Z

Woohoo! Thanks, my pleasure!

uki00a · 2020-07-08T17:14:25Z

Released in v0.11.0!

Terkwood · 2020-07-08T19:51:17Z

So cool!! Thanks!

Terkwood mentioned this pull request Jun 8, 2020

Stream API #36

Closed

Implement xadd and xread

2c82632

Terkwood force-pushed the feat-streams branch from 17ac7d1 to 2c82632 Compare June 8, 2020 01:21

Terkwood added 27 commits June 7, 2020 22:02

Implement xdel

3e55b82

Implement xlen

c5b6805

Rewrite xaddMaxlen

993f81e

Rework arg

c70de2b

Use snake case for consistency

3b65989

Update test

2e0bcb6

Add TODO

55bf5d5

Add test

6410b22

Trim

90d59fc

Merge branch 'master' into feat-streams

733f651

Add method sig

d2979d9

Draft XREADGROUP impl

3dd3647

STUB TEST

43166c5

add test

f8b1758

stub out method

3e64b6b

fix sig

43c4176

Add impls

436913c

Add docs

9a73e34

Add docs and impl

9962324

Assert

5dae056

Assert

9572768

stub test

5145e15

add tests

00af089

Update interface

b7d6180

Update test

d7d3f87

Add xack and xreadgroup auto ack

2d3630c

Stub tests

9d6082a

Terkwood added 5 commits June 30, 2020 08:20

Implement methods

944c088

Expand test

e122375

Expand test

eacc608

Finish test

e1aef2d

Match doc params to method sigs

cc7738f

Terkwood marked this pull request as ready for review June 30, 2020 14:33

Merge remote-tracking branch 'upstream/master' into feat-streams

1eebbc3

uki00a added this to the v0.11.0 milestone Jun 30, 2020

uki00a self-assigned this Jun 30, 2020

uki00a requested changes Jul 1, 2020

View reviewed changes

Terkwood and others added 7 commits July 2, 2020 11:36

Fix scoping of cleanupStream

4cd274b

Simplify test assertions

16b736b

Update command.ts

c71a318

Co-authored-by: uki00a <uki00a@gmail.com>

Use delay in test

c2c3738

Convert to assertThrowsAsync...

7e08289

...but leaves the error type and message undefined.

Trim log statement

024ddc9

Fix text

0e6a2fd

uki00a requested changes Jul 2, 2020

View reviewed changes

tests/stream_test.ts Outdated Show resolved Hide resolved

tests/stream_test.ts Show resolved Hide resolved

Fix test assertion

32820a3

uki00a approved these changes Jul 2, 2020

View reviewed changes

uki00a merged commit b6e5f3d into denodrivers:master Jul 2, 2020

uki00a mentioned this pull request Jul 2, 2020

[Feedback wanted] Return type for XREAD/XREADGROUP #109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏞️ Implement streams commands #86

🏞️ Implement streams commands #86

Terkwood commented Jun 7, 2020 •

edited

Loading

uki00a commented Jun 30, 2020

uki00a Jun 30, 2020

Terkwood Jul 2, 2020

Terkwood Jul 2, 2020 •

edited

Loading

Terkwood Jul 2, 2020

uki00a Jul 1, 2020

uki00a Jul 1, 2020

uki00a commented Jul 1, 2020 •

edited

Loading

Terkwood commented Jul 1, 2020

uki00a left a comment

uki00a commented Jul 2, 2020

uki00a commented Jul 2, 2020

Terkwood commented Jul 2, 2020

uki00a commented Jul 8, 2020

Terkwood commented Jul 8, 2020

🏞️ Implement streams commands #86

🏞️ Implement streams commands #86

Conversation

Terkwood commented Jun 7, 2020 • edited Loading

Add stream commands

API design 🎨

Strongly Typed Message IDs

Return type for XREAD 📕

Allow input of record objects in XADD

Commands to implement and test

docs and sanity checks

special reply types to model

Additional requirements

Always Remember

uki00a commented Jun 30, 2020

uki00a Jun 30, 2020

Choose a reason for hiding this comment

Terkwood Jul 2, 2020

Choose a reason for hiding this comment

Terkwood Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Terkwood Jul 2, 2020

Choose a reason for hiding this comment

uki00a Jul 1, 2020

Choose a reason for hiding this comment

uki00a Jul 1, 2020

Choose a reason for hiding this comment

uki00a commented Jul 1, 2020 • edited Loading

Terkwood commented Jul 1, 2020

uki00a left a comment

Choose a reason for hiding this comment

uki00a commented Jul 2, 2020

uki00a commented Jul 2, 2020

Terkwood commented Jul 2, 2020

uki00a commented Jul 8, 2020

Terkwood commented Jul 8, 2020

Terkwood commented Jun 7, 2020 •

edited

Loading

Terkwood Jul 2, 2020 •

edited

Loading

uki00a commented Jul 1, 2020 •

edited

Loading