Make INFO command variadic by hwware · Pull Request #6891 · redis/redis

hwware · 2020-02-14T22:00:00Z

The problem/use-case that the feature addresses

This is an enhancement for INFO command, previously INFO only support one argument for different info section , if user want to get more categories information, either perform INFO all/default or calling INFO for multiple times.

Description of the feature

The goal of adding this feature is to let the user retrieve multiple categories via the INFO command, and still avoid emitting the same section twice.

A use case for this is like Redis Sentinel, which periodically calling INFO command to refresh info from monitored Master/Slaves, only Server and Replication part categories are used for parsing information.
If the INFO command can return just enough categories that client side needs, it can save a lot of time for client side parsing it as well as network bandwidth.

Implementation
To share code between redis, sentinel, and other users of INFO (DEBUG and modules), we have a new genInfoSectionDict function that returns a dict and some boolean flags (e.g. all) to the caller (built from user input).
Sentinel is later purging unwanted sections from that, and then it is forwarded to the info genRedisInfoString.

Usage Examples
INFO Server Replication
INFO CPU Memory
INFO default commandstats

itamarhaber

I like the enhancement, but I'm not sure about the implementation style. Also, there are some edge cases, for example, INFO default all will return "all".

hwware · 2020-02-15T14:03:36Z

@itamarhaber Hi Itamar, thanks for your comment, the INFO default all return "INFO all" I was considering this since we need to return upperbound information that user needed. If user specifies "all" then we can simplely return every category. Thanks

hwware · 2021-10-13T19:37:47Z

@yossigo, Can you please take a look at this PR. This is a old PR but I think it is still useful and valid. I have rebased the code for this PR to remove any conflicts. Thanks

oranagra

i always wanted to do that, so i do support the cause, but i don't like the implementation.
it's nice that the implementation attempts to make a small diff by repeatedly calling genRedisInfoString rather than modifying all the section checks in that function, but i think we have to go that way anyway since the current approach is limited.

another thing to consider is backwards compatibility, in the sense of what would happen if someone tries to call INFO memory replication on an older version of redis.
calling it with multiple arguments (or a single argument with spaces in it) will fail, so for many tools that manage redis servers and can't be sure which version they're talking to, using this feature is not an option.

src/server.c

hwware · 2021-11-02T19:40:57Z

Hey @oranagra, I have updated the code for this PR and editted the top comment for description. Please take a look.
Thank you.

oranagra

@hwware sorry about the delay.
i must say i don't like this approach.
the default sections are now defined in two places (both inside genRedisInfoString and in the list in infoCommand).

normally, it has two loops on all the arguments (in infoCommand), and then another loop on the dict it generates.
and it calls genRedisInfoString multiple times, each call ends up doing many string compares.

what's nice about your approach is that you managed to do it without a single change to genRedisInfoString, but i don't think that's a real concern.

what i imagine should be done instead, is pass the dict to genRedisInfoString (calling it only once), and change all the section checks in genRedisInfoString to use dictFind.

similar logic needs to be implemented in RM_InfoAddSection (i already designed the API with that feature of multiple sections in mind).

i think the two loops on argc can be unified into one, which will result in a dict and two boolean flags (all and everything, which will be passed to genRedisInfoString as argument along side the dict).

the list of default sections will be present only once in the source code (in infoCommand), and will be added to the dict in case no arguments are provided, or the term "default" was specified.

let me know what you think.

p.s. your current approach won't play nice with modules, since RM_InfoAddSection can include a certain section both by module name filter and section name filter (so there's a change the same section will appear twice if it is present in the dict twice)

hwware · 2021-11-15T20:24:30Z

Hey @oranagra, I agree with your suggesions. I have some questions about the implementation.

The dictionary which is passed to the genRedisInfoString function, should we add default to it or all the subsections of default like server, memory, cpu ,etc.
Should we unify the two loops for argc? The first one is used to check if default is present and sets the has_def_sections flag to 1. This is used to avoid duplication in the case info cpu default is called. The first loop won't we needed if instead of default, we add the individual components (server, cpu, memory, ...) of default to the dict.

oranagra · 2021-11-16T07:00:01Z

yes, the loop will add the individual default sections (cpu, memory, etc) to the dict when it sees the argument default mentioned, or when no arguments are passed.
the arguments we pass to genRedisInfoString will be: the dict, and the all and everything flags.

src/module.c

src/server.c

src/sentinel.c

oranagra

@hwware are you still working on this PR?
It seems you where either unable to follow my advises or run into some problems that i didn't expect and can't figure out.

do you wanna proceed or want me to try pick it up on my own?
after investing the effort so far, i think we should carry it though.

src/debug.c

src/sentinel.c

src/server.c

hwware · 2021-12-13T14:30:24Z

@hwware are you still working on this PR? It seems you where either unable to follow my advises or run into some problems that i didn't expect and can't figure out.

do you wanna proceed or want me to try pick it up on my own? after investing the effort so far, i think we should carry it though.

Hi Oran, I am still working on this. I will let you know when my codes update. Thanks a lot.

hwware · 2021-12-20T19:55:00Z

@oranagra Hi Oran, I just finish the code refactor according to your suggestion. You could find the following changes:

create a new function named: genSectionDict, it is used to create a dictionary
in the modulesCollectInfo, i pass the whole dict as parameter.
Please take a look and let me know any concern. Thanks a lot for your advises.

oranagra

i took a quick look.
i didn't review the diff from last time (too much back and forth)
but rather re-read the diff of the current version vs unstable.
it's a little bit closer to what i suggested, but there are a few bugs, styling issues, and most importantly the handling of sentinel default sections and requested sections sanitization isn't what we agreed.

i assume all the old comments can be dismissed now (please go over them and dismiss them unless you notice some observation that was forgotten)

p.s. i suppose some tests are needed for both the happy path, and some edge cases.

src/server.c

src/debug.c

src/module.c

src/debug.c

src/module.c

src/server.c

* Update test cases * Minor changes * Update 05-manual.tcl * Update 10-replica-priority.tcl

* Update test cases for sentinel (#12) * Update test cases * Minor changes * Update 05-manual.tcl * Update 10-replica-priority.tcl * Update 07-down-conditions.tcl * Update ci.yml * Update ci.yml * Update to test 7 and ci * Update 07-down-conditions.tcl * Delete 07-down-conditions.tcl * Create 07-down-conditions.tcl * Update 07-down-conditions.tcl

hwware · 2022-02-01T21:32:54Z

@hwware it seemed that i'm unable to communicate my design suggestion, so i went ahead and just implemented it. also resolved some bugs, and the rest of my unresolved comments. merged recent unstable and made sure the tests are passing. please review my changes and let me know if you see any problem.

p.s. i'm not sure the sentinel tests are good enough to detect any issues, so i suppose they better be improved. also, i think we should not rely on the module tests alone, and add some tests to the normal redis test suite.

@oranagra As we discussed, I update the c->argv+2 and c->argc-2 to c->argv+1 and c->argc-1 to fix the minor bug, now in sentinel mode, "info" command run well, and I add more test cases for "info", "info all", "info default" and "info everything" commands, and I also add some test cases for "info + one subsection or multiple sections" for redis server mode and sentinel mode. Please take a look and Thanks for your great help.

tests/sentinel/tests/13-info-command.tcl

tests/unit/info-command.tcl

tests/unit/moduleapi/infotest.tcl

oranagra · 2022-02-03T09:10:35Z

@redis/core-team please approve, changing INFO to take multiple sections.

yossigo

LGTM, one potential optimization to consider.

src/server.c

itamarhaber

LGTM

oranagra · 2022-02-06T17:07:23Z

I've benchmarked this PR against unstable, with the plain INFO command (no args).
even with the recent optimization of not creating the default dict every time, it's still about 10 times slower.
i've tried adding an out_defaults argument, and adding an defaults || to all the sections ifs (so we don't resort to dictFind on each one), but it didn't help.
it seems that the remaining calls to dictFind (e.g. for commandstats and latencystats) are causing the slowdown.
and it also seems that the latency is the same if we have just one of call to dictFind or several of them.
calling dictExpand didn't help.

oranagra · 2022-02-06T19:21:06Z

scrap all that, the regression was due to the accidental inclusion of the latencystats section by default (not the use of dict).

branch	throughput	latency
unstable	52994	0.873
w/o dict caching	47961	0.971
w/ dict caching	54347	0.851
avoid most dictFind	54854	0.842
with latencystats	6664	7.432

oranagra · 2022-02-06T19:43:43Z

my final version:

args	throughput	latency
no args	53390	0.867
"default"	49701	0.936
"server"	137931	0.229
explicitly list all default sections	47596	0.979
"all"	6565	7.539

unstable:

args	throughput	latency
no args	49652	0.936
"default"	49212	0.942
"server"	140646	0.200
"all"	6876	7.192

madolson

Approve of the API, just some minor comments.

src/server.c

Introduced in redis#6891

* Fix INFO SENTINEL memory leak Introduced in #6891 * remove the copy-paste sentence

itamarhaber reviewed Feb 15, 2020

View reviewed changes

hwware marked this pull request as draft October 13, 2021 18:13

hwware force-pushed the infoenhancement branch 2 times, most recently from 189d87e to f8d8d17 Compare October 13, 2021 18:47

hwware marked this pull request as ready for review October 13, 2021 19:19

hwware requested review from oranagra and yossigo October 13, 2021 19:21

oranagra reviewed Oct 14, 2021

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

oranagra mentioned this pull request Nov 2, 2021

[NEW] INFO Command Enhancement: Return Different Categories Combination to User #9660

Open

hwware force-pushed the infoenhancement branch from 859d242 to 422596f Compare November 2, 2021 18:20

oranagra reviewed Nov 15, 2021

View reviewed changes

hwware mentioned this pull request Nov 16, 2021

Info new hwware/redis#42

Closed

hwware force-pushed the infoenhancement branch 2 times, most recently from a9e4b4e to f99bb3a Compare November 18, 2021 21:37

oranagra reviewed Nov 21, 2021

View reviewed changes

oranagra added the 7.0-not-after-rc1 label Dec 7, 2021

oranagra reviewed Dec 13, 2021

View reviewed changes

src/debug.c Outdated Show resolved Hide resolved

src/sentinel.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

oranagra reviewed Dec 26, 2021

View reviewed changes

oranagra reviewed Dec 31, 2021

View reviewed changes

src/debug.c Outdated Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

hwware changed the title ~~INFO Command Enhancement: Return Different Categories Combination to User~~ WIP: INFO Command Enhancement: Return Different Categories Combination to User Jan 4, 2022

hwware changed the title ~~WIP: INFO Command Enhancement: Return Different Categories Combination to User~~ WIP INFO Command Enhancement: Return Different Categories Combination to User Jan 4, 2022

hwware added 2 commits January 6, 2022 11:29

Update test cases for sentinel (#12)

4f57f5e

* Update test cases * Minor changes * Update 05-manual.tcl * Update 10-replica-priority.tcl

oranagra reviewed Feb 2, 2022

View reviewed changes

oranagra added approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Feb 3, 2022

hwware and others added 2 commits February 3, 2022 15:42

update test cases

45c6405

fix leak and cleanup tests

a3c463d

oranagra approved these changes Feb 4, 2022

View reviewed changes

yossigo approved these changes Feb 5, 2022

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

cache the default section dict

766caaa

itamarhaber approved these changes Feb 6, 2022

View reviewed changes

Merge remote-tracking branch 'origin/unstable' into infoenhancement

c686867

oranagra added 4 commits February 6, 2022 21:27

avoid dictFind

64d8523

remove latencystats from defaults

a404ce1

undo last commit, except dictExpand and fix for out_everything

445980d

bugfix

3cee214

yossigo approved these changes Feb 6, 2022

View reviewed changes

madolson approved these changes Feb 7, 2022

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

oranagra changed the title ~~INFO Command Enhancement: Return Different Categories Combination to User~~ Make INFO command variadic Feb 8, 2022

use case insensitive compare instead of sdstolower

63ef80b

oranagra merged commit 2e1bc94 into redis:unstable Feb 8, 2022

enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Feb 9, 2022

Fix INFO SENTINEL memory leak

42f3ed0

Introduced in redis#6891

enjoy-binbin mentioned this pull request Feb 9, 2022

Fix INFO SENTINEL memory leak #10268

Merged

oranagra pushed a commit that referenced this pull request Feb 9, 2022

Fix INFO SENTINEL memory leak (#10268)

beb94c9

* Fix INFO SENTINEL memory leak Introduced in #6891 * remove the copy-paste sentence

oranagra mentioned this pull request Feb 28, 2022

Release 7.0 RC2 #10355

Merged

hwware mentioned this pull request Mar 1, 2022

Enhancement SENTINEL MASTERS | MASTER | REPLICAS | SENTINELS command #10158

Closed

Conversation

hwware commented Feb 14, 2020 • edited by oranagra 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!

hwware commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hwware commented Oct 13, 2021

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hwware commented Nov 2, 2021

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

hwware commented Nov 15, 2021

Uh oh!

oranagra commented Nov 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hwware commented Dec 13, 2021

Uh oh!

hwware commented Dec 20, 2021

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hwware commented Feb 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Feb 3, 2022

Uh oh!

yossigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itamarhaber left a comment

Choose a reason for hiding this comment

hwware commented Feb 14, 2020 •

edited by oranagra

Loading

hwware commented Feb 15, 2020 •

edited

Loading

oranagra commented Feb 6, 2022 •

edited

Loading