Add support for `SPUBLISH` by atakavci · Pull Request #2838 · redis/lettuce

atakavci · 2024-04-24T06:49:56Z

Closes/Fixes #2757

implementation is similar to PUBLISH.
using existing routing mechanism.
some of commented/disabled tests related to SSUBSCRIBE are enabled regarding to TODO notes.

You have read the contribution guidelines.
You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.

codecov · 2024-04-24T06:55:54Z

Codecov Report

Attention: Patch coverage is 63.33333% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 77.64%. Comparing base (43843bf) to head (b906da7).
Report is 252 commits behind head on main.

Files	Patch %	Lines
...io/lettuce/core/cluster/PubSubClusterEndpoint.java	60.00%	2 Missing ⚠️
...ore/cluster/pubsub/RedisClusterPubSubListener.java	0.00%	2 Missing ⚠️
...va/io/lettuce/core/pubsub/RedisPubSubListener.java	0.00%	2 Missing ⚠️
...io/lettuce/core/AbstractRedisReactiveCommands.java	0.00%	1 Missing ⚠️
...core/cluster/pubsub/RedisClusterPubSubAdapter.java	0.00%	1 Missing ⚠️
...ava/io/lettuce/core/pubsub/RedisPubSubAdapter.java	0.00%	1 Missing ⚠️
...e/core/pubsub/RedisPubSubReactiveCommandsImpl.java	0.00%	1 Missing ⚠️
.../api/coroutines/BaseRedisCoroutinesCommandsImpl.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2838      +/-   ##
============================================
- Coverage     78.71%   77.64%   -1.07%     
- Complexity     6786     7238     +452     
============================================
  Files           508      539      +31     
  Lines         22765    24519    +1754     
  Branches       2446     2607     +161     
============================================
+ Hits          17919    19039    +1120     
- Misses         3717     4277     +560     
- Partials       1129     1203      +74

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.java

mp911de · 2024-04-24T09:30:38Z

src/test/java/io/lettuce/core/cluster/pubsub/RedisClusterPubSubConnectionIntegrationTests.java

+        pubSubConnection.async().ssubscribe(shardChannel);
+        Wait.untilEquals(shardChannel, connectionListener.getShardChannels()::poll).waitOrTimeout();
+
+        pubSubConnection.async().ssubscribe(shardTestChannel);


It would make sense to have a test that operates on a single shard connection to ensure that no routing (MOVED redirection) happens.

Cluster connections transparently handle redirects.

could you eloborate it ?
are you looking for additional tests where we just use node listener instead of connection listener ?

I just realized that we cannot easily test MOVED redirections unless setting ClusterClientOptions.maxRedirects to 0.

That would be in any case worth testing for Pub/Sub connections to open a new connection and make sure that no redirects happen to ensure maximum performance.

In my first comment, I mixed up things so please disregard the "single shard connection" aspect.

LMK if this covers what you have in mind.

tishun

LGTM

src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.java

src/test/java/io/lettuce/core/cluster/pubsub/RedisClusterPubSubConnectionIntegrationTests.java

…nTests

* implementation of SPUBLISH * sort methods by name * test cluster spublish with no redirects * use injected cluster client in RedisClusterPubSubConnectionIntegrationTests

@SInCE

* Remove not readonly commands #2832 * Fix comment Add test * Update test * Update cicd.yaml workaround for apt update issue related to Clearsigned file * GitHub issue template polishing and stale issues action (#2833) * GitHub issue template polishing and stale issues action * Addressing feedback on message test and a better label for the stale issues * Implement HEXPIRE, HEXPIREAT, HEXPIRETIME and HPERSIST (#2836) * HEXPIRE implemented with integration tests * Polishing to integration test, added unit test * Move new commands to RedisHashCommands, added HEXPIREAT, HEXPIRETIME and HPERSIST * Make sure we reset the configuration setting after the new hash commands were tested * Broke one test because of wrong configuration setting; the other is unstable, trying to fix it * Polishing imports * Polishin : Copyright change not needed * MAke sure we wait for the check so it does not fail on the slower pipeline (#2844) * Add support for `SPUBLISH` (#2838) * implementation of SPUBLISH * sort methods by name * test cluster spublish with no redirects * use injected cluster client in RedisClusterPubSubConnectionIntegrationTests * Applying code formatter each time we run a Maven build (#2841) * Let's try this again * Add tests and templates * Merge formatting issues + formatter using wrong configuration * Update cicd.yaml the issue related apt repo has been fixed https://github.com/orgs/community/discussions/120966#discussioncomment-9211925 * Add support CLIENT KILL [MAXAGE] (#2782) * Add support CLIENT KILL [MAXAGE] * polish * address review changes * format code * Add support for `SUNSUBSCRIBE` #2759 (#2851) * Add support for `SUNSUBSCRIBE` #2759 * replace junit.Assert with assertj * Mark dnsResolver(DnsResolver) as deprecated. (#2855) * Implement HPEXPIRE, HPEXPIREAT, HPEXPIRETIME, HTTL and HPTTL (#2857) * Fixes to the hash field expiration functions * Implemented HPEXPIRE, HPEXPIREAT, HPEXPIRETIME, HTTL, HPTTL * Format the files * FIELDS keyword was introduced as per the PRD * Extend tests to include HTTL * Repair unit tests, add new ones for the new commands * Disable failing test, becasue it is very unstable * Modify return value to list of long values, fix cluster logic * Polishing * Polishing - addressed Ali's comments * Polishing: Modified by mistake * Polishing : Addressed Marks' comments * XREAD support for reading last message from stream (#2863) * Add last() utility method to the XArgs.StreamOffset * Submitted one more file by mistake * Add a `evalReadOnly` overload that accepts the script as a `String` (#2868) * Add a evalReadOnly overload that accepts the script as a String * Fix @SInCE annotation for 7.0 * Add release drafter workflow (#2820) * Release-drafter, dependabot, OCD polishing * Commitish not needed and pointing to the wrong branch anyway, also wrong version chosen * Pipeline is also very confusing as there are many pipelines, this is the INTEGRATION pipeline * Adding commitish, as it is required for filter-by-commitish * Autolabeling hits issues when using pull_request, using pull_request_target instead * pull_request_target does nothing, trying the other suggestion from release-drafter/release-drafter#1125 * Bump org.apache.maven.plugins:maven-javadoc-plugin from 3.6.3 to 3.7.0 (#2873) Bumps [org.apache.maven.plugins:maven-javadoc-plugin](https://github.com/apache/maven-javadoc-plugin) from 3.6.3 to 3.7.0. - [Release notes](https://github.com/apache/maven-javadoc-plugin/releases) - [Commits](apache/maven-javadoc-plugin@maven-javadoc-plugin-3.6.3...maven-javadoc-plugin-3.7.0) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-javadoc-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump org.codehaus.mojo:flatten-maven-plugin from 1.5.0 to 1.6.0 (#2874) Bumps [org.codehaus.mojo:flatten-maven-plugin](https://github.com/mojohaus/flatten-maven-plugin) from 1.5.0 to 1.6.0. - [Release notes](https://github.com/mojohaus/flatten-maven-plugin/releases) - [Commits](mojohaus/flatten-maven-plugin@1.5.0...1.6.0) --- updated-dependencies: - dependency-name: org.codehaus.mojo:flatten-maven-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump org.apache.maven.plugins:maven-jar-plugin from 3.3.0 to 3.4.1 (#2875) Bumps [org.apache.maven.plugins:maven-jar-plugin](https://github.com/apache/maven-jar-plugin) from 3.3.0 to 3.4.1. - [Release notes](https://github.com/apache/maven-jar-plugin/releases) - [Commits](apache/maven-jar-plugin@maven-jar-plugin-3.3.0...maven-jar-plugin-3.4.1) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-jar-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump org.openjdk.jmh:jmh-generator-annprocess from 1.21 to 1.37 (#2876) Bumps [org.openjdk.jmh:jmh-generator-annprocess](https://github.com/openjdk/jmh) from 1.21 to 1.37. - [Commits](openjdk/jmh@1.21...1.37) --- updated-dependencies: - dependency-name: org.openjdk.jmh:jmh-generator-annprocess dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump org.apache.commons:commons-pool2 from 2.11.1 to 2.12.0 (#2877) Bumps org.apache.commons:commons-pool2 from 2.11.1 to 2.12.0. --- updated-dependencies: - dependency-name: org.apache.commons:commons-pool2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Setting the next release to be 6.4.x as part of #2880 (#2881) * Bump version of lettuce-core to 6.4.0 * Change all @SInCE notations to match the next release 6.4.x and also fixed two typos in the API JavaDoc * Fixed formatting issues * Modify the release acrtion to call the proper maven target for release, make releasing manually available too (#2885) * Format file * Using interface method from java 8 instead of java 9 * Fix test * Revert readwrite command to the list COMMAND INFO READWRITE 1) 1) "readwrite" 2) "1" 3) 1) "loading" 2) "stale" 3) "fast" --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: atakavci <[email protected]> Co-authored-by: Tihomir Krasimirov Mateev <[email protected]> Co-authored-by: Liming Deng <[email protected]> Co-authored-by: Wang Zhi <[email protected]> Co-authored-by: Luis Miguel Mejía Suárez <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

jkrauze · 2024-08-22T14:59:46Z

src/main/java/io/lettuce/core/pubsub/RedisPubSubReactiveCommandsImpl.java


+    @Override
+    public Mono<Long> spublish(K shardChannel, V message) {
+        return createMono(() -> commandBuilder.publish(shardChannel, message));


this should point to spublish, not publish

created a bug issue: #2971

implementation of SPUBLISH

dd7be34

atakavci requested review from gerzse, mp911de and tishun April 24, 2024 06:49

mp911de approved these changes Apr 24, 2024

View reviewed changes

sort methods by name

247287a

atakavci requested a review from mp911de April 24, 2024 11:03

tishun approved these changes Apr 24, 2024

View reviewed changes

test cluster spublish with no redirects

b906da7

tishun assigned atakavci Apr 24, 2024

tishun added the type: feature A new feature label Apr 24, 2024

tishun mentioned this pull request Apr 25, 2024

Applying code formatter each time we run a Maven build #2841

Merged

4 tasks

use injected cluster client in RedisClusterPubSubConnectionIntegratio…

1f3d86d

…nTests

atakavci merged commit dfbdddc into redis:main May 2, 2024

tishun modified the milestones: 7.0.0.RELEASE, 6.4.0.RELEASE Jun 6, 2024

jkrauze reviewed Aug 22, 2024

View reviewed changes

atakavci deleted the ali/lettuce/issue-2757 branch February 27, 2026 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `SPUBLISH`#2838

Add support for `SPUBLISH`#2838
atakavci merged 4 commits intoredis:mainfrom
atakavci:ali/lettuce/issue-2757

atakavci commented Apr 24, 2024

Uh oh!

codecov bot commented Apr 24, 2024 •

edited

Loading

Uh oh!

Uh oh!

mp911de Apr 24, 2024

Uh oh!

atakavci Apr 24, 2024

Uh oh!

mp911de Apr 24, 2024

Uh oh!

atakavci Apr 24, 2024

Uh oh!

tishun left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkrauze Aug 22, 2024

Uh oh!

jkrauze Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

atakavci commented Apr 24, 2024

Uh oh!

codecov bot commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

mp911de Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

atakavci Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

mp911de Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

atakavci Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

tishun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkrauze Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

jkrauze Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Apr 24, 2024 •

edited

Loading