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

Connecting to cluster example #2298

Merged

Conversation

varadkarpe
Copy link
Contributor

Adding an example to connect to an open source Redis cluster.
This issue is described in detail in :
#2281 (comment)

@lgtm-com
Copy link

lgtm-com bot commented Oct 18, 2022

This pull request introduces 1 alert when merging 80e1c8b into d0bfa77 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@SuzeShardlow
Copy link
Contributor

Closes #2281 when 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.

@varadkarpe thanks for your PR, I've noted a few areas where it doesn't meet the contribution guidelines (see https://github.com/redis/node-redis/tree/master/examples#readme)

Please also add your example script to the table in the README, keeping the scripts in alphabetical order.

@@ -0,0 +1,32 @@
//This is an example script to connect to a running cluster.
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 is an example script to connect to a running cluster.
// This is an example script to connect to a running cluster.

@@ -0,0 +1,32 @@
//This is an example script to connect to a running cluster.
// After connecting to the cluster I am attempting to set and get a value.
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
// After connecting to the cluster I am attempting to set and get a value.
// After connecting to the cluster the code then sets and gets a value.

//This is an example script to connect to a running cluster.
// After connecting to the cluster I am attempting to set and get a value.

//To setup this cluster you can follow the guide here :
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
//To setup this cluster you can follow the guide here :
// To setup this cluster you can follow the guide here:

//To setup this cluster you can follow the guide here :
// https://redis.io/docs/manual/scaling/
// In this guide the ports which are being used are 7000 - 7005
//But since I am a macOS user I am using 7001 - 7006
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
//But since I am a macOS user I am using 7001 - 7006

Removed as not needed.


//To setup this cluster you can follow the guide here :
// https://redis.io/docs/manual/scaling/
// In this guide the ports which are being used are 7000 - 7005
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
// In this guide the ports which are being used are 7000 - 7005

Removed as not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt it was important to mention the port numbers which I am using since the master nodes in the example script are using the ports 7001 - 7003.

await cluster.connect();

await cluster.set('foo', 'bar');
const value = await cluster.get('foo')
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
const value = await cluster.get('foo')
const value = await cluster.get('hello');

Added ; as the guidelines for examples say to use them - see README in the examples folder.


await cluster.connect();

await cluster.set('foo', 'bar');
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
await cluster.set('foo', 'bar');
await cluster.set('hello', 'cluster');

Note that the guidelines in the README in the examples folder say not to use foo and bar examples.


await cluster.set('foo', 'bar');
const value = await cluster.get('foo')
console.log(value)
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(value)
console.log(value);
await cluster.quit();

Adds missing ;, also quits as per the example script requirements in the README.

@varadkarpe
Copy link
Contributor Author

Hey @simonprickett
I have pushed the changes as per the suggestions made. I have a doubt in regard to the suggestion made in regard to the port numbers:
Is it not necessary for me to mention the port numbers which I am using since the master nodes in this particular example happen to be operating on 7001 - 7003 but in the example script which I have referred for the setup the master nodes operate on 7000 - 7002?

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2022

Codecov Report

Base: 95.85% // Head: 93.42% // Decreases project coverage by -2.42% ⚠️

Coverage data is based on head (da98acb) compared to base (d0bfa77).
Patch has no changes to coverable lines.

❗ Current head da98acb differs from pull request most recent head 900d1bb. Consider uploading reports for the commit 900d1bb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2298      +/-   ##
==========================================
- Coverage   95.85%   93.42%   -2.43%     
==========================================
  Files         433      433              
  Lines        4002     4002              
  Branches      451      451              
==========================================
- Hits         3836     3739      -97     
- Misses        102      202     +100     
+ Partials       64       61       -3     
Impacted Files Coverage Δ
packages/client/lib/commands/FUNCTION_LOAD.ts 16.66% <0.00%> (-83.34%) ⬇️
packages/client/lib/commands/FUNCTION_FLUSH.ts 20.00% <0.00%> (-80.00%) ⬇️
packages/client/lib/commands/FUNCTION_RESTORE.ts 20.00% <0.00%> (-80.00%) ⬇️
packages/client/lib/commands/CLIENT_NO-EVICT.ts 33.33% <0.00%> (-66.67%) ⬇️
packages/client/lib/commands/FUNCTION_LIST.ts 37.50% <0.00%> (-62.50%) ⬇️
packages/client/lib/commands/FUNCTION_STATS.ts 25.00% <0.00%> (-62.50%) ⬇️
packages/client/lib/commands/LCS_IDX.ts 50.00% <0.00%> (-50.00%) ⬇️
packages/client/lib/commands/LCS_LEN.ts 50.00% <0.00%> (-50.00%) ⬇️
packages/client/lib/commands/SINTERCARD.ts 50.00% <0.00%> (-50.00%) ⬇️
packages/client/lib/commands/ZINTERCARD.ts 50.00% <0.00%> (-50.00%) ⬇️
... and 24 more

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 1eed12e into redis:master Oct 19, 2022
@SuzeShardlow
Copy link
Contributor

@varadkarpe thanks for your contribution. If you are a member of our Discord server (discord.gg/redis) please let us know your handle so we can give you the Hacktoberfest 2022 role.

@varadkarpe
Copy link
Contributor Author

Hey @SuzeShardlow
Thank you for the invite, I have joined the discord server, my handle is varad.karpe#5500

@SuzeShardlow
Copy link
Contributor

Thanks, I've given you the role! :)

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