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

ISPN-14648 GETDEL Command #10877

Merged
merged 1 commit into from
May 11, 2023
Merged

ISPN-14648 GETDEL Command #10877

merged 1 commit into from
May 11, 2023

Conversation

rigazilla
Copy link
Contributor

@rigazilla rigazilla commented Apr 28, 2023

@rigazilla rigazilla marked this pull request as ready for review May 2, 2023 11:45
Copy link
Contributor

@karesti karesti left a comment

Choose a reason for hiding this comment

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

GEt DEL is not in this PR but append ...

@rigazilla
Copy link
Contributor Author

GEt DEL is not in this PR but append ...

GEt DEL is not in this PR but append ...

mmm, maybe a mees with the rebase

@rigazilla rigazilla force-pushed the ISPN-14648 branch 2 times, most recently from 1b63b78 to fb477c7 Compare May 2, 2023 14:57
@rigazilla
Copy link
Contributor Author

based on #10876 for assertj

ChannelHandlerContext ctx,
List<byte[]> arguments) {
if (arguments.size() != getArity()-1) {
ByteBufferUtils.stringToByteBuf("-ERR wrong number of arguments for 'append' command\r\n", handler.allocatorToUse());
Copy link
Member

Choose a reason for hiding this comment

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

I would centralize all -ERR strings into a Messages class using a MessageBundle (see the CLI).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, but I would trace it in a separate issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

public CompletionStage<RespRequestHandler> perform(Resp3Handler handler,
ChannelHandlerContext ctx,
List<byte[]> arguments) {
if (arguments.size() != getArity()-1) {
Copy link
Member

Choose a reason for hiding this comment

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

formatting seems a bit off here (spaces around the subtraction)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in the APPEND PR, this depends on #10876 for assertj

Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

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

LGTM, just minor change

@@ -88,8 +89,7 @@ public String getName() {
// DEL should always be first here
indexedRespCommand[3] = new RespCommand[]{new DEL(), new DECR(), new DECRBY()};
indexedRespCommand[4] = new RespCommand[]{new ECHO()};
// GET should always be first here
Copy link
Member

Choose a reason for hiding this comment

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

Don't delete this comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ouch 🙂 fixed

@rigazilla rigazilla force-pushed the ISPN-14648 branch 2 times, most recently from c644018 to 5d33b5e Compare May 9, 2023 10:47
Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

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

LGTM, however CI didn't build on last run, maybe it needs a rebase?

@rigazilla
Copy link
Contributor Author

rebased, let's see...

@rigazilla rigazilla requested a review from karesti May 11, 2023 07:59
@karesti karesti merged commit 2e6e106 into infinispan:main May 11, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants