Xinfo command #2069

MichalCho · 2019-10-01T09:29:57Z

Hi,

I've noticed that xinfo command is missing so I've started to implement it.
This is just to start a discussion about how this should be fixed.
Currently I've implemented a single xinfo command that returns a map of <String,Objects>.
This would work for any variant of xinfo (stream,groups, consumers) but it's a bit generic.

The other idea would be to create 3 xinfo commands, returning something like XInfoStream, XInfoConsumers etc. objects. That would give a better visibility of returned values.
I will be happy to work on it.

Builder classes used for decoding Returned type changed to Map<String,Object>

…o_command

MichalCho · 2019-10-01T09:51:39Z

@sazzad16 @marcosnils @gkorland @xetorthio could you give me some input?
Thanks

sazzad16 · 2019-10-01T13:23:30Z

My initial thought is that Params is a better fit for this.

MichalCho · 2019-10-01T13:42:30Z

My initial thought is that Params is a better fit for this.

But this will not solve the problem of returning very generic object.
jedis.xinfo(myKey, STREAM) and jedis.xinfo(myKey, CONSUMERS, myGroup) would return the same type - just a map. Then user would to need to know the implementation to actually check the returned values example:

jedis.xinfo(myKey, STREAM).get("first-entry); - they would need to know that this is a StreamElement and do casting in their code.

What I had in mind is to have 3 functions returning different types which expose more clear getters

XStreamInfo streamInfo = jedis.xinfo(myKey, STREAM); //Type specific to information about a stream
streamInfo.getFirstEntry(); //returned type is StreamElement, no need for casting;

Other option would be to return something like XInfoElement that contains 3 fields:

XInfoStream
XinfoGroups
XInfoConsumers

exposing getXInfoStream, getXInfoGroups and XInfoConsumers.
They would be set when a specific info is requested. That would require if-not-null check.

gkorland · 2019-10-15T21:38:35Z

Sorry my mistake... should be draft

MichalCho · 2019-10-16T10:16:27Z

Sorry my mistake... should be draft

No problem, I'm working on the new implementation, I hope I can have some commits ready soon

gkorland · 2019-12-18T13:45:15Z

@MichalCho sorry to nudge, but are still working on this PR?

MichalCho · 2019-12-18T20:33:49Z

@gkorland no problem. Sorry I got quite busy at the office. I will try to use holiday break now to craft some code. Will you be willing to do some code review?

gkorland · 2019-12-19T11:19:26Z

@MichalCho no pressure, I just wanted to check if it's still relevant for you.
Sure, once you finalize the PR I'll find some time to review it

startjava · 2019-12-24T05:55:14Z

@MichalCho sorry,,,i want know xinfo() method Progress？？ :) :) :D

MichalCho · 2019-12-26T12:35:41Z

@MichalCho sorry,,,i want know xinfo() method Progress？？ :) :) :D

Hi @startjava
I will try to have some commits ready for review after New Year's Eve.
Are you waiting for this feature? :)

startjava · 2019-12-26T13:13:10Z

@MichalCho
not waiting this feature，But Always waiting this feature :）:) :D

A lot of refactoring to be done. This commit is just for showing the idea

MichalCho · 2019-12-29T17:41:52Z

Hi,
@gkorland I've just pushed my initial idea how to implement this. It require a lot more testing (I've just created one happy case test case). Do you think you could have a look just at it to give me feedback what do you think about usability? Maybe my approach is not the best, I will be happy to discuss it. Then I can continue to work on it after New Year and finalize PR according to your feedback
Thanks

src/main/java/redis/clients/jedis/BuilderFactory.java

src/main/java/redis/clients/jedis/Jedis.java

src/main/java/redis/clients/jedis/BinaryClient.java

sazzad16 · 2019-12-30T06:58:38Z

@MichalCho Even though I've put some review comments, ~~I'm liking~~* I'm not entirely liking the idea. Are we complicating it too much? As the Redis documentation for XINFO says

a well behaving client should fetch the whole list, and report it to the user, for example, as a dictionary data structure

Perhaps one method (per class) returning a Map is enough? @gkorland WDYT?

BTW, sorry about typo which reversed what I tried to mean.

MichalCho · 2020-01-03T11:14:26Z

@sazzad16 thanks a lot for the review. Regarding formatting comments - I was aware of most of them, I just wanted to show something before leaving for holidays :)

@MichalCho Even though I've put some review comments, I'm liking the idea. Are we complicating it too much? As the Redis documentation for XINFO says

a well behaving client should fetch the whole list, and report it to the user, for example, as a dictionary data structure

Perhaps one method (per class) returning a Map is enough? @gkorland WDYT?

I was thinking about it. Then I had an idea to provide getters for all map members. Otherwise user will have to try to get on Object from the map and he will have to know what kind of object it is.

something like (if xinfo returns just a map):
Long streamLenght info = (Long)jedis.xinfo("xadd-stream1", StreamInfo.StreamInfoType.getStreamInfoType()).get("lenght");

Another issue that I had in mind is - when we decode the answer we can not be fully flexible as we need to know how to decode the answer (to know that "length" is a integer, "last-generated-id" is a string and "first-entry" is a stream entry object).
One solution would be to try to decode first element that is a name of a field (let's say "length") and then try to decode next element using known type (let's say first try to decode as a string, if we fail, then try to decode as stream object, then as a integer etc) until we succeed. If we fail we throw some kind of decoding exception.
I think that would be the only way to be fully transparent and able to react on changes done on redis side.
I will try to fix the stuff you have commented on and also craft my new approach for decoding an answer from redis. in few days. In the meantime if you have some thought about it, please let me know.

startjava · 2020-01-06T08:05:17Z

@MichalCho hi~ what time xinfo method final over? thank you job !

xinfo commands return now just a Map<String,Object> In case when (in the future) redis includes additional fields in a reply we will try to decode them using known decoders

MichalCho · 2020-01-09T18:11:49Z

hi @sazzad16!

I've implemented your comments and changed the returned type just to be Map<String,Object>

During the weekend I will try to add more test cases and maybe make the code prettier.
Looking forward to any comments from you and @gkorland
Thanks

PS @startjava it's ongoing, you can check the latest commit

Some code clean up done together with more tests

MichalCho · 2020-01-13T16:51:17Z

Hi @sazzad16 and @gkorland

I've added more tests and worked on the review comments. I think that the PR is ready for a final review from you guys.

I have one thing in mind - how should we act if redis decide to add more fields in a reply and we are not able to decode them? Ignore them and decode what we can? Throw an exception? Log something?
So far I just silently ignore elements that are not possible to decode by any of known Builder used for that type of info (stream,group, consumer).

Please share your thoughts.
Thanks!

src/main/java/redis/clients/jedis/BuilderFactory.java

src/test/java/redis/clients/jedis/tests/commands/StreamsCommandsTest.java

src/main/java/redis/clients/jedis/BinaryClient.java

src/main/java/redis/clients/jedis/BuilderFactory.java

src/main/java/redis/clients/jedis/StreamGroupInfo.java

Some minor refactoring done together with restoring getters for elements known to be returned by redis now. In case new elements are added in the future the map can be used to obtain them.

MichalCho · 2020-01-14T10:14:40Z

I've just committed changes according to review comments.

I've restored getters for map elements, that are known to be present in a reply as per today.
In case some more elements are added in the future they can still be accessed through the map.

But please check my reply to your comment - should we have those getters or just keep constants (not variables) that can make getting objects from the map easier.

Still the question is - how shall we try to decode those unknown elements. As for now we try use a set of decoding functions that we know are needed to decode specific type of answer. Should all decoding function should be added?
Anyway I feel we are getting closer to finalize that PR

Refactor xinfo to simplify usage. Javadoc will be added in another commit

MichalCho · 2020-01-31T12:16:33Z

hi @gkorland,

I've implemented your suggestion - splitting xinfo into 3 different methods. I've also added some simple javadocs to make it easier for a user to understand how to use xinfo with jedis.

I had some doubts regarding how I create info objects, for example:

if (map!= null) {
     streamInfo = map;
     length = (Long) map.get(LENGTH);

This can fail if length is not present in the map( for example future version of redis will remove this filed). But according to redis documentation:

Note that you should not rely on the fields exact position, nor on the number of fields, new fields may be added in the future.

so I guess it's save to assume that fields that exist now will always be there. Nothing is mentioned about removing fields.

sazzad16 · 2020-02-02T14:59:05Z

src/main/java/redis/clients/jedis/commands/BinaryJedisCommands.java

+  byte[] xinfoStream (byte[] key);
+
+  List<byte[]> xinfoGroup (byte[] key);
+
+  List<byte[]> xinfoConsumers (byte[] key, byte[] group);


Why byte[]? What about the Objects?

I think we need some aligment here. I can see that some BinaryJedisCommands returns Objects (or Collections of Objects) - in some cases using builder factory directly like

public List<GeoRadiusResponse> georadiusByMember(final byte[] key, final byte[] member, final double radius, final GeoUnit unit, final GeoRadiusParam param) { checkIsInMultiOrPipeline(); client.georadiusByMember(key, member, radius, unit, param); return BuilderFactory.GEORADIUS_WITH_PARAMS_RESULT.build(client.getObjectMultiBulkReply()); }

some other returns byte arrays or collection of byte arrays etc.
Maybe I missunderstood the purpose of binary client - I thought it was supposed to use just binary data, without trying to decode them. Could you please help me to understand that?
I have seen in #2084 that you mention that returning List<byte[]> instead of List is a bug

If we should return objects then I would propose following:

Object xinfoStream (byte[] key); List<Object> xinfoGroup (byte[] key); List<Object> xinfoConsumers (byte[] key, byte[] group);

Following would be good enough:

StreamInfo xinfoStream (byte[] key); List<StreamGroupInfo> xinfoGroup (byte[] key); List<StreamConsumersInfo> xinfoConsumers (byte[] key, byte[] group);

I've refactored it that way and added a test for binary usage.

sazzad16 · 2020-02-02T15:04:25Z

src/main/java/redis/clients/jedis/Protocol.java

+  public static final String STREAM = "STREAM";
+  public static final String GROUPS = "GROUPS";
+  public static final String CONSUMERS = "CONSUMERS";


Prefix these, perhaps with XINFO_. Also put these along with SENTINEL_..., CLUSTER_..., PUBSUB_... variables.

Also a test for binary xinfo commands has been added

MichalCho · 2020-02-11T08:26:50Z

Pinging @sazzad16 @gkorland :)

gkorland · 2020-02-11T09:15:50Z

src/test/java/redis/clients/jedis/tests/commands/StreamsCommandsTest.java

+    List<StreamConsumersInfo> consumersInfo = jedis.xinfoConsumers(STREAM_NAME, G1);
+
+    //Stream info test
+    assertEquals(2L,streamInfo.getStreamInfo().get(LENGTH));


Missing LAST_GENERATED_ID

Done in a way that we check if it's not null - it is difficult to know what last-generated should be

shouldn't it be id2?

you are right. I've missed that we have that info already.

I have also changed returned object to be StreamEntryID

src/test/java/redis/clients/jedis/tests/commands/StreamsCommandsTest.java

src/main/java/redis/clients/jedis/Protocol.java

src/main/java/redis/clients/jedis/StreamInfo.java

gkorland · 2020-02-11T09:27:48Z

src/main/java/redis/clients/jedis/StreamInfo.java

+     firstEntry = (StreamEntry) map.get(FIRST_ENTRY);
+     lastEntry = (StreamEntry) map.get(LAST_ENTRY);
+
+    } else throw  new IllegalArgumentException("InfoMap can not be null");


If we are throwing runtime exception anyway when map is null isn't it better to just let it throw NullPointerException?

Done. My initial thought was that it is not nice to see NullPointers

gkorland · 2020-02-11T09:27:54Z

src/main/java/redis/clients/jedis/StreamGroupInfo.java

+     pending = (long) map.get(PENDING);
+     lastDeliveredId = (String) map.get(LAST_DELIVERED);
+
+    } else throw new IllegalArgumentException();


If we are throwing runtime exception anyway when map is null isn't it better to just let it throw NullPointerException?

Done. My initial thought was that it is not nice to see NullPointers

gkorland · 2020-02-11T09:28:06Z

src/main/java/redis/clients/jedis/StreamConsumersInfo.java

+      pending = (long) map.get(PENDING);
+
+
+    } else throw new IllegalArgumentException();


If we are throwing runtime exception anyway when map is null isn't it better to just let it throw NullPointerException?

Done. My initial thought was that it is not nice to see NullPointers

Added test for last-generated-id Moved xinfo keywords to enum Removed throwing runtime exception from xinfo classes

Better to use existing class

gkorland · 2020-02-11T14:01:32Z

Tests are failing https://travis-ci.org/xetorthio/jedis/jobs/648852944

MichalCho · 2020-02-11T14:42:52Z

Tests are failing https://travis-ci.org/xetorthio/jedis/jobs/648852944

I'm on it

Update:
in test environment, if everything is executed fast consumersInfo.get(0).getIdle() returns sometimes 0 - as this is info returned by server.
In the test we create a consumer and then check for its details.
Easy fix would be just to introduce some delay between creating a consumer and reading it's state - like with simple Thread.sleep() - what do you think @gkorland?

gkorland · 2020-02-12T08:48:56Z

@MichalCho sounds OK just sleep for 1ms

LastDeliveredId type has been changed to be StreamEntryId instead of simple String. Also unstable test for Idle Consumer time has been fixed.

MichalCho · 2020-02-12T12:30:13Z

I think we are done here. @gkorland is there a list of features/ issues to be implemented/fixed? I would like to contribute more :)

sazzad16 · 2020-02-12T12:53:36Z

Merged into master. Excellent job @MichalCho

MichalCho · 2020-02-13T09:21:37Z

Thanks,
@gkorland @sazzad16 do we have a list of features/ issues to be implemented/fixed? I would like to contribute more?

gkorland · 2020-02-13T09:44:04Z

@MichalCho thanks for the great help!
You can see here the tasks marked as features https://github.com/xetorthio/jedis/labels/FEATURE

Michal-Cho added 3 commits September 10, 2019 11:32

Initial commit for XINFO command

f3750c3

Refactor xinfo stream command

7178775

Builder classes used for decoding Returned type changed to Map<String,Object>

Merge branch 'master' of https://github.com/xetorthio/jedis into xinf…

047c257

…o_command

gkorland marked this pull request as ready for review October 15, 2019 21:36

Happy case implementation of xinfo

0094384

A lot of refactoring to be done. This commit is just for showing the idea

sazzad16 requested changes Dec 30, 2019

View reviewed changes

Refactor of xinfo after review comments

5d051dd

xinfo commands return now just a Map<String,Object> In case when (in the future) redis includes additional fields in a reply we will try to decode them using known decoders

Add more tests

2690e01

Some code clean up done together with more tests

MichalCho requested a review from sazzad16 January 13, 2020 16:10

sazzad16 requested changes Jan 14, 2020

View reviewed changes

Restore getters for map elements

059190e

Some minor refactoring done together with restoring getters for elements known to be returned by redis now. In case new elements are added in the future the map can be used to obtain them.

MichalCho requested a review from sazzad16 January 14, 2020 10:14

Michal-Cho added 2 commits January 31, 2020 09:57

Refactor xinfo method into 3 separate

dc9cf2f

Refactor xinfo to simplify usage. Javadoc will be added in another commit

Update javadocs

76a4b63

MichalCho requested a review from gkorland January 31, 2020 12:16

gkorland requested a review from sazzad16 February 2, 2020 14:51

sazzad16 requested changes Feb 2, 2020

View reviewed changes

Refactor binary jedis xinfo commands

a442fe6

Also a test for binary xinfo commands has been added

MichalCho requested a review from sazzad16 February 4, 2020 14:53

sazzad16 approved these changes Feb 11, 2020

View reviewed changes

gkorland suggested changes Feb 11, 2020

View reviewed changes

Small fixes after review

6f6c88f

Added test for last-generated-id Moved xinfo keywords to enum Removed throwing runtime exception from xinfo classes

MichalCho requested a review from gkorland February 11, 2020 10:15

Michal-Cho added 2 commits February 11, 2020 12:00

Update last_generated_id test

daa90be

Change getLastGeneratedId to return StreamEntryID

df4255d

Better to use existing class

gkorland approved these changes Feb 11, 2020

View reviewed changes

gkorland added ready to merge and removed wait for more reviews labels Feb 11, 2020

Update StreamGroupInfo and test

c50cf79

LastDeliveredId type has been changed to be StreamEntryId instead of simple String. Also unstable test for Idle Consumer time has been fixed.

gkorland approved these changes Feb 12, 2020

View reviewed changes

sazzad16 merged commit 528debd into redis:master Feb 12, 2020

sazzad16 removed the ready to merge label Feb 12, 2020

MichalCho deleted the xinfo_command branch February 13, 2020 16:51

		pending = (long) map.get(PENDING);


		} else throw new IllegalArgumentException();

Xinfo command #2069

Xinfo command #2069

Conversation

MichalCho commented Oct 1, 2019

MichalCho commented Oct 1, 2019 • edited

sazzad16 commented Oct 1, 2019

MichalCho commented Oct 1, 2019

gkorland commented Oct 15, 2019

MichalCho commented Oct 16, 2019

gkorland commented Dec 18, 2019

MichalCho commented Dec 18, 2019

gkorland commented Dec 19, 2019 • edited

startjava commented Dec 24, 2019

MichalCho commented Dec 26, 2019

startjava commented Dec 26, 2019

MichalCho commented Dec 29, 2019 • edited

sazzad16 commented Dec 30, 2019 • edited

MichalCho commented Jan 3, 2020

startjava commented Jan 6, 2020

MichalCho commented Jan 9, 2020

MichalCho commented Jan 13, 2020 • edited

MichalCho commented Jan 14, 2020 • edited

MichalCho commented Jan 31, 2020

Choose a reason for hiding this comment

MichalCho Feb 4, 2020 • edited

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

MichalCho commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalCho Feb 11, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkorland commented Feb 11, 2020

MichalCho commented Feb 11, 2020 • edited

gkorland commented Feb 12, 2020

MichalCho commented Feb 12, 2020 • edited

sazzad16 commented Feb 12, 2020

MichalCho commented Feb 13, 2020

gkorland commented Feb 13, 2020 • edited

MichalCho commented Oct 1, 2019 •

edited

gkorland commented Dec 19, 2019 •

edited

MichalCho commented Dec 29, 2019 •

edited

sazzad16 commented Dec 30, 2019 •

edited

MichalCho commented Jan 13, 2020 •

edited

MichalCho commented Jan 14, 2020 •

edited

MichalCho Feb 4, 2020 •

edited

MichalCho Feb 11, 2020 •

edited

MichalCho commented Feb 11, 2020 •

edited

MichalCho commented Feb 12, 2020 •

edited

gkorland commented Feb 13, 2020 •

edited