GEODE-9831: support SISMEMBER support command #7164
Conversation
Kris-10-0
requested review from
DonalEvans,
dschneider-pivotal,
jdeppe-pivotal,
kirklund,
nonbinaryprogrammer,
ringles and
upthewaterspout
as code owners
.../org/apache/geode/redis/internal/commands/executor/set/AbstractSIsMemberIntegrationTest.java
Outdated
Show resolved
Hide resolved
.../org/apache/geode/redis/internal/commands/executor/set/AbstractSIsMemberIntegrationTest.java
Outdated
Show resolved
Hide resolved
Kris-10-0
force-pushed
the
feature/GEODE-9831
branch
from
da3d5d8 to
1211b12
Compare
|
I accidentally forced push my new commit, but the changes on the commit were to fix a typo and remove the concurrency test. There was no modification on the ordering of the commits. |
| SADD(new SAddExecutor(), SUPPORTED, new Parameter().min(3).flags(WRITE, DENYOOM, FAST)), | ||
| SDIFF(new SDiffExecutor(), SUPPORTED, | ||
| new Parameter().min(2).lastKey(-1).flags(READONLY, SORT_FOR_SCRIPT)), | ||
| SISMEMBER(new SIsMemberExecutor(), SUPPORTED, new Parameter().exact(3).flags(READONLY, FAST)), |
There was a problem hiding this comment.
I'd recommend writing a simple unit test for RedisCommandType. It would just confirm that SISMEMBER is supported and maybe has an SIsMemberExecutor and 3 parameters including the READONLY and FAST flags.
There was a problem hiding this comment.
The correctness of the RedisCommandType flags is tested and validated against native Redis in CommandIntegrationTest, and the parameters are tested in the sismemberWrongNumberOfArguments_returnsError test in AbstractSIsMemberIntegrationTest, so we have comprehensive coverage of this code.
5 participants
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
[NA] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?