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

OSS Broadcast support #3285

Merged
merged 12 commits into from
Feb 27, 2023
Merged

OSS Broadcast support #3285

merged 12 commits into from
Feb 27, 2023

Conversation

sazzad16
Copy link
Contributor

@sazzad16 sazzad16 commented Feb 1, 2023

No description provided.

@sazzad16 sazzad16 marked this pull request as draft February 1, 2023 06:13
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2023

Codecov Report

Base: 66.93% // Head: 66.98% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (a4ea008) compared to base (1d898f1).
Patch coverage: 66.17% of modified lines in pull request are covered.

❗ Current head a4ea008 differs from pull request most recent head 42a038b. Consider uploading reports for the commit 42a038b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3285      +/-   ##
============================================
+ Coverage     66.93%   66.98%   +0.04%     
- Complexity     4641     4650       +9     
============================================
  Files           258      263       +5     
  Lines         15027    15053      +26     
  Branches        938      949      +11     
============================================
+ Hits          10059    10083      +24     
+ Misses         4565     4563       -2     
- Partials        403      407       +4     
Impacted Files Coverage Δ
...in/java/redis/clients/jedis/ConnectionFactory.java 67.34% <ø> (ø)
...ients/jedis/JedisBroadcastAndRoundRobinConfig.java 0.00% <0.00%> (ø)
...rc/main/java/redis/clients/jedis/JedisCluster.java 32.43% <ø> (ø)
...rc/main/java/redis/clients/jedis/JedisFactory.java 64.15% <ø> (ø)
src/main/java/redis/clients/jedis/Protocol.java 89.50% <0.00%> (-2.37%) ⬇️
...a/redis/clients/jedis/DefaultRedisCredentials.java 41.17% <41.17%> (ø)
...ain/java/redis/clients/jedis/RedisCredentials.java 50.00% <50.00%> (ø)
...rc/main/java/redis/clients/jedis/UnifiedJedis.java 74.84% <52.94%> (-0.45%) ⬇️
...ents/jedis/exceptions/JedisBroadcastException.java 52.94% <52.94%> (ø)
...lients/jedis/executors/ClusterCommandExecutor.java 80.00% <65.21%> (-5.08%) ⬇️
... and 11 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.

@sazzad16 sazzad16 marked this pull request as ready for review February 1, 2023 06:52
@sazzad16
Copy link
Contributor Author

sazzad16 commented Feb 1, 2023

@uglide

@chayim
Copy link
Contributor

chayim commented Feb 1, 2023

@sazzad16 WDYT about adding examples (and a jedis-examples pom) with this PR? This would be the first one, but IMHO for a PR like this, it's nice, because it's easier to see the usage of the implementation?

@sazzad16
Copy link
Contributor Author

sazzad16 commented Feb 1, 2023

@sazzad16 WDYT about adding examples (and a jedis-examples pom) with this PR? This would be the first one, but IMHO for a PR like this, it's nice, because it's easier to see the usage of the implementation?

@chayim It's not possible due to the current structure of this repo. We'd have to restructure it with a main parent directory which would contain several directories, i.e. jedis-examples, as well as jedis the current main directory.

@chayim
Copy link
Contributor

chayim commented Feb 1, 2023

@chayim It's not possible due to the current structure of this repo. We'd have to restructure it with a main parent directory which would contain several directories, i.e. jedis-examples, as well as jedis the current main directory.

I mean we have src/main and src/tests, why not src/examples, and exclude it from the package...? This can also be out of scope. Alternatively we add an examples tree at the top, with at least a single file end-to-ending it as part of this.

@chayim chayim changed the title Broadcast - approach ii OSS Broadcast support Feb 5, 2023
@sazzad16
Copy link
Contributor Author

sazzad16 commented Feb 5, 2023

@chayim

we have src/main and src/tests

Those directories are following maven standard.

why not src/examples

That's not part of maven standard. You still can if you want but that wouldn't be following any standard directory structure.

Alternatively we add an examples tree at the top, with at least a single file end-to-ending it as part of this.

I don't understand.

@sazzad16
Copy link
Contributor Author

sazzad16 commented Feb 7, 2023

@uglide @chayim PING

chayim
chayim previously approved these changes Feb 7, 2023
@sazzad16 sazzad16 requested a review from chayim February 8, 2023 09:41
@sazzad16 sazzad16 added this to the 4.4.0 milestone Feb 8, 2023
@sazzad16 sazzad16 requested a review from a team February 14, 2023 06:16
@chayim
Copy link
Contributor

chayim commented Feb 14, 2023

@uglide after everything we discussed, need your feedback here. I've already had discussions with @sazzad16

@uglide
Copy link
Contributor

uglide commented Feb 27, 2023

@sazzad16 @chayim LGTM. I can't wait to see support for JSON.MGET support in broadcasted mode and round-robin for TS.MGET and FT.SEARCH.

uglide
uglide previously approved these changes Feb 27, 2023
@sazzad16 sazzad16 merged commit 6d60ed6 into redis:master Feb 27, 2023
@sazzad16 sazzad16 deleted the broad-2 branch February 27, 2023 15:59
banker pushed a commit to banker/jedis that referenced this pull request Mar 28, 2023
* OSS Cluster Broadcast - Approach 2

* modify

* Added doc

* Added configSet

* Added example as Java class

* Added exception message from code review

* Added the link of examples package in README

* Deleted doc

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

Successfully merging this pull request may close these issues.

4 participants