Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#2287 Add example scripts showing pub/sub usage. #2288

Merged
merged 4 commits into from Oct 14, 2022

Conversation

con-mark
Copy link
Contributor

@con-mark con-mark commented Oct 5, 2022

Description

Added example requested for Issue #2287

created pubsub-publisher.js and pubsub-subscriber.js


Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@SuzeShardlow
Copy link
Contributor

Closes #2287 when approved and merged.

Copy link
Contributor

@simonprickett simonprickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there - when I run pubsub-subscriber.js node-redis errors because the client isn't connected... I thjink you need to add await client.connect(); in there to connect to Redis before trying any Redis commands.

I would also suggest having the publisher console log what it's doing, and finally please ensure that the table of example scripts in README has its formatting maintained so that the | lines up.

These are just minor things - nice work and we look forward to merging.

@SuzeShardlow
Copy link
Contributor

@con-mark thanks for your PR - not sure if you have seen @simonprickett's comments above, but he is asking for a couple of changes before we merge this. Thanks!

Fixing comments requested
Adding client.connect() to pubsub-subscriber.js
Reformatting Readme
updating logging in pubsub-publisher.js
@con-mark
Copy link
Contributor Author

con-mark commented Oct 6, 2022

Hi @simonprickett @SuzeShardlow I have made requested changes please let me know if any further changes are needed.

@SuzeShardlow
Copy link
Contributor

Thanks @con-mark, we will take a look in our morning! :)

Copy link
Contributor

@simonprickett simonprickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some tidy up commit suggestions, but am also unable to get the example to work, the publisher publishes messages but the subscriber never outputs anything...

for (let i = 0; i < 10000; i++) {
//1st channel created to publish 10000 messages
await client.publish(channel1, `channel1_message_${i}`);
console.log(`publising message on ${channel1}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(`publising message on ${channel1}`);
console.log(`publishing message on ${channel1}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

console.log(`publising message on ${channel1}`);
//2nd channel created to publish 10000 messages
await client.publish(channel2, `channel2_message_${i}`);
console.log(`publising message on ${channel2}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(`publising message on ${channel2}`);
console.log(`publishing message on ${channel2}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


await client.connect();

//declare constant variables for the name of the clients we will publish to as they will be required for logging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//declare constant variables for the name of the clients we will publish to as they will be required for logging
// Declare constant variables for the name of the clients we will publish to as they will be required for logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

const channel2 = 'chan2nel';

for (let i = 0; i < 10000; i++) {
//1st channel created to publish 10000 messages
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//1st channel created to publish 10000 messages
// 1st channel created to publish 10000 messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

//1st channel created to publish 10000 messages
await client.publish(channel1, `channel1_message_${i}`);
console.log(`publising message on ${channel1}`);
//2nd channel created to publish 10000 messages
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//2nd channel created to publish 10000 messages
// 2nd channel created to publish 10000 messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

const client = createClient();
await client.connect();

//Each subscriber needs to connect individually therefore we duplicate the client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//Each subscriber needs to connect individually therefore we duplicate the client.
// Each subscriber needs to connect individually therefore we duplicate the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

await noChannelsSub.connect();
await allChannelsSub.connect();

//This subscriber only will receive messages from channel 1 as they are using the subscribe method and subscribed to chan1nel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//This subscriber only will receive messages from channel 1 as they are using the subscribe method and subscribed to chan1nel
// This subscriber only will receive messages from channel 1 as they are using the subscribe method and subscribed to chan1nel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

console.log(`Channel1 subscriber collected message: ${message}`);
},true);

//This subscriber only will receive messages from channel 2 as they are using the subscribe method and subscribed to chan2nel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//This subscriber only will receive messages from channel 2 as they are using the subscribe method and subscribed to chan2nel
// This subscriber only will receive messages from channel 2 as they are using the subscribe method and subscribed to chan2nel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

console.log(`Channel2 subscriber collected message: ${message}`);
},true);

//This subscriber receive messages from both channel 1 and channel 2 thanks the pSubscribe method.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//This subscriber receive messages from both channel 1 and channel 2 thanks the pSubscribe method.
// This subscriber receive messages from both channel 1 and channel 2 using the pSubscribe method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

console.log(`Channel ${channel} sent message: ${message}`);
},true);

//clean up after ourselves by unsubsribing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//clean up after ourselves by unsubsribing
// Clean up after ourselves by unsubscribing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed clean up

@SuzeShardlow
Copy link
Contributor

@con-mark, please see the above comments from @simonprickett and let us know if you can make the changes - the subscriber in your example isn't outputting anything for us. Thanks!

Fix publish and subscriber
Update tidy up comments
@con-mark
Copy link
Contributor Author

con-mark commented Oct 8, 2022

Hi @SuzeShardlow and @simonprickett I have made the tidy up comments and I have fixed the issue with the subscriber seems like doing the clean up and closing the client before they got a chance to interact with the Redis to publish or subscribe.

Please let me know if you need anything more.

I also may suggest some other improvement for your hacktoberfest contribution might be for someone to create a docker compose that will allow the the running of the examples with a redis image for people to not have to worry for set up :)

Copy link
Contributor

@simonprickett simonprickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@con-mark thanks - left one tiny suggestion and this should then be good to go.

examples/pubsub-subscriber.js Outdated Show resolved Hide resolved
Making suggested changes

Co-authored-by: Simon Prickett <simon@crudworks.org>
@con-mark
Copy link
Contributor Author

Hi @SuzeShardlow @simonprickett is any further changes needed here? Please let me know :)

@codecov-commenter
Copy link

Codecov Report

Base: 95.85% // Head: 95.85% // No change to project coverage 👍

Coverage data is based on head (27bbbb4) compared to base (2a8e11a).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2288   +/-   ##
=======================================
  Coverage   95.85%   95.85%           
=======================================
  Files         433      433           
  Lines        4002     4002           
  Branches      451      451           
=======================================
  Hits         3836     3836           
  Misses        102      102           
  Partials       64       64           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@SuzeShardlow SuzeShardlow merged commit d0bfa77 into redis:master Oct 14, 2022
@SuzeShardlow
Copy link
Contributor

@con-mark thanks for your contribution! If you are a member of our Discord server, please let us know your handle so we can give you the Hacktoberfest 2022 role. Thanks!

@con-mark
Copy link
Contributor Author

Hi @SuzeShardlow my discord is dev-mark#1608

@SuzeShardlow
Copy link
Contributor

Thanks @con-mark, I've given you the role in Discord :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants