Extend client tracking tests by nitaicaro · Pull Request #7998 · redis/redis

nitaicaro · 2020-10-30T06:53:46Z

Test support for the new map, null and push message types. Map objects are parsed as a list of lists of key value pairs.
for instance: user => john password => 123

will be parsed to the following TCL list:

{{user john} {password 123}}

Also added the following tests:

Redirection still works with RESP3
Able to use a RESP3 client as a redirection client
No duplicate invalidation messages when turning BCAST mode on after normal tracking
Server is able to evacuate enough keys when num of keys surpasses limit by more than defined initial effort
Different clients using different protocols can track the same key
OPTOUT tests
OPTIN tests
Clients can redirect to the same connection
tracking-redir-broken test
HELLO 3 checks
Invalidation messages still work when using RESP3, with and without redirection
Switching to RESP3 doesn't disturb previous tracked keys
Tracking info is correct
Flushall and flushdb produce invalidation messages

These tests achieve 100% line coverage for tracking.c using lcov.

itamarhaber

Wow, thanks for this. My TCL isn't what it used to be, so this LGTM :)

oranagra

WOW. thank you @nitaicaro.
i skimmed over the changes and added a few random comments.

maybe later, someone need to have another careful review at the tests to check they're not missing anything.
i.e. i understand the C code is now 100% covered, that's great, but maybe some validations in the tests side can be improved to verify correct behavior (i.e like the issue #7871 fixed).

tests/support/redis.tcl

tests/unit/tracking.tcl

Added test support for the new map, null and push message types. map objects are parsed as a list of lists of key value pairs. for instance: user => john password => 123 will be parsed to the following TCL list: {{user john} {password 123}} Added the following tests: * Redirection still works with RESP3 * Able to use a RESP3 client as a redirection client * No duplicate invalidation messages when turning BCAST mode on after normal tracking * Server is able to evacuate enough keys when num of keys surpasses limit by more than defined initial effort * Different clients using different protocols can track the same key * OPTOUT tests * OPTIN tests * Clients can redirect to the same connection * tracking-redir-broken test * HELLO 3 checks * Invalidation messages still work when using RESP3, with and without redirection * Switching to RESP3 doesn't disturb previous tracked keys * Tracking info is correct These tests achieve 100% line coverage for tracking.c using lcov.

oranagra · 2020-11-01T21:39:08Z

@nitaicaro you did a force push containing a rebase, and some changes of your own?
please avoid that, i have no way to see what you changed.
it's fine to edit your code and force push (github will let me diff the commits), but if you also rebased in the same push, the changes are being mixed.
i suppose in this case the rebase didn't bring any changes to tracking.tcl, and redis.tcl, so all the changes there are yours.

i skimmed over the changes, i see you reduced the use of clean_all to just 2 places, the rest of the (both old and new) test just incrementally build on top of each other.
and i see you reverted the key name changes.
anything else you wanna point out for an easier review?

please temporarily modify .github/workflows/daily.yml to run on your PR (possibly adding --single unit/tracking to all the ./runtest lines, so we can see how it works on all the slower platforms and valgrind.

p.s. it seems you have a conflict, i thought the rebase was in order to solve it. not sure what went wrong.

nitaicaro · 2020-11-02T07:29:00Z

Sorry about that @oranagra and noted.

I also changed the way the new NULL type is parsed. You can see the difference in redis.tcl. I also fixed a test that was acting a bit funny (RESP3 Client gets tracking-redir-broken push message after cached key changed when rediretion client is terminated) and removed the sleep from the Server is able to evacuate enough keys when num of keys surpasses limit by more than defined initial effort test.

oranagra · 2020-11-02T15:38:47Z

@nitaicaro
you'll need to remove this part from the daily yaml:

branches:
      # any PR to a release branch.	      # any PR to a release branch.
      - '[0-9].[0-9]'

and if you intend to test it on your own repo (pushing to another branch rather than the one used for this PR. i.e. if you're gonna run a lot of tests and don't wanna bother us seeing them), you'll need to remove this:

if: github.repository == 'redis/redis'

the reason i'm worried is that with deferred clients, you can't assume that by the time tcl reached the next line, redis actually processed the command you sent to it.
you may need something like this to make "sure" it was executed.

$rd flush
after 100

or better yet, some wait_for_condition loop looking at the flags of the CLIENT list.
possibly adding a CLIENT INFO (like the one discussed in #8000 and flags that discussed in #7995)

madolson · 2020-11-02T19:26:00Z

+1 about the thankyou for adding tests. It's boring work people usually don't like doing 👍

The one function I see that is "covered" but not validated is flushall/flushdb send invalidation messages.

oranagra · 2020-11-04T20:55:28Z

it looks like there are no test timing issues that jump out. maybe we'll just have to deal with them when they come.

@nitaicaro can you please validate the correct behavior of flushdb that @madolson pointed out.
maybe @eliblight can review the tests and come up with other things that should / could be validated.

madolson · 2020-11-04T21:03:03Z

@nitaicaro and @eliblight work together btw. He's already reviewed it.

eliblight · 2020-11-05T13:36:17Z

indeed i have

…

On Wed, 4 Nov 2020, 23:03 Madelyn Olson, ***@***.***> wrote: @nitaicaro <https://github.com/nitaicaro> and @eliblight <https://github.com/eliblight> work together btw. He's already reviewed it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7998 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYF35VXAJROCVY6ETBCBBDSOG6RPANCNFSM4TET5HNQ> .

Added test support for the new map, null and push message types. map objects are parsed as a list of lists of key value pairs. for instance: user => john password => 123 will be parsed to the following TCL list: {{user john} {password 123}} Added the following tests: - Redirection still works with RESP3 - Able to use a RESP3 client as a redirection client - No duplicate invalidation messages when turning BCAST mode on after normal tracking - Server is able to evacuate enough keys when num of keys surpasses limit by more than defined initial effort - Different clients using different protocols can track the same key - OPTOUT tests - OPTIN tests - Clients can redirect to the same connection - tracking-redir-broken test - HELLO 3 checks - Invalidation messages still work when using RESP3, with and without redirection - Switching to RESP3 doesn't disturb previous tracked keys - Tracking info is correct - Flushall and flushdb produce invalidation messages These tests achieve 100% line coverage for tracking.c using lcov.

…stable

nitaicaro · 2020-11-08T13:08:04Z

Implemented your suggestions @oranagra.

oranagra · 2020-11-09T19:06:27Z

@nitaicaro tests are passing (at least 2 more times), i guess we're good to merge this.
please revert the changes to the daily CI, and see if the top comment of the PR needs any update.
then we can squash-merge it (using the top comment as the commit comment)
thank you.

This reverts commit 6f44e20.

This reverts commit d6d45cc.

…stable

nitaicaro · 2020-11-09T20:12:15Z

@oranagra done

madolson · 2020-11-09T22:30:16Z

I did glance through the difference from the last review, and didn't see anything wrong.

Test support for the new map, null and push message types. Map objects are parsed as a list of lists of key value pairs. for instance: user => john password => 123 will be parsed to the following TCL list: {{user john} {password 123}} Also added the following tests: Redirection still works with RESP3 Able to use a RESP3 client as a redirection client No duplicate invalidation messages when turning BCAST mode on after normal tracking Server is able to evacuate enough keys when num of keys surpasses limit by more than defined initial effort Different clients using different protocols can track the same key OPTOUT tests OPTIN tests Clients can redirect to the same connection tracking-redir-broken test HELLO 3 checks Invalidation messages still work when using RESP3, with and without redirection Switching to RESP3 doesn't disturb previous tracked keys Tracking info is correct Flushall and flushdb produce invalidation messages These tests achieve 100% line coverage for tracking.c using lcov.

itamarhaber previously approved these changes Oct 31, 2020

View reviewed changes

oranagra reviewed Nov 1, 2020

View reviewed changes

oranagra mentioned this pull request Nov 1, 2020

add tracking bcast flag and client redirection in client list #7995

Merged

nitaicaro dismissed itamarhaber’s stale review via e966194 November 1, 2020 13:28

Merge branch 'unstable' into unstable

5f1ae0a

added --single unit/tracking to daily.yml

d6d45cc

Update daily.yml

6f44e20

nitaicaro and others added 3 commits November 8, 2020 13:04

Merge branch 'unstable' of https://github.com/nitaicaro/redis into un…

415c66c

…stable

Update tracking.tcl

23f7fa4

Nitai Caro added 3 commits November 9, 2020 20:06

Revert "Update daily.yml"

39ee338

This reverts commit 6f44e20.

Revert "added --single unit/tracking to daily.yml"

e0b8b6b

This reverts commit d6d45cc.

Merge branch 'unstable' of https://github.com/nitaicaro/redis into un…

952c806

…stable

oranagra approved these changes Nov 9, 2020

View reviewed changes

oranagra changed the title ~~New tracking tests~~ Extend client tracking tests Nov 9, 2020

oranagra merged commit 19c29b6 into redis:unstable Nov 9, 2020

Conversation

nitaicaro commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamarhaber left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Nov 1, 2020

Uh oh!

nitaicaro commented Nov 2, 2020

Uh oh!

oranagra commented Nov 2, 2020

Uh oh!

madolson commented Nov 2, 2020

Uh oh!

oranagra commented Nov 4, 2020

Uh oh!

madolson commented Nov 4, 2020

Uh oh!

eliblight commented Nov 5, 2020 via email

Uh oh!

nitaicaro commented Nov 8, 2020

Uh oh!

oranagra commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nitaicaro commented Nov 9, 2020

Uh oh!

madolson commented Nov 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nitaicaro commented Oct 30, 2020 •

edited

Loading

oranagra commented Nov 9, 2020 •

edited

Loading