Skip to content

Conversation

@vladvildanov
Copy link
Contributor

@vladvildanov vladvildanov commented Jan 26, 2024

Closes #1422

This PR changes on-connection behaviour by using fewer commands during handshake

Additional enhancements:

  • Updated CommandInterface with serializeCommand() method that returns RESP representation of command. According to Information Expert pattern Command object should provides it's serialized representation instead of Connection object.
  • Updated NodeConnectionInterface with write() method that writes serialized command directly into resource. This method existed before but was protected.
  • Updated NodeConnectionInterface with getClientId() method that returns CLIENT ID assigned by Redis server for current connection.

@coveralls
Copy link

coveralls commented Jan 26, 2024

Coverage Status

coverage: 92.534% (+0.1%) from 92.436%
when pulling 66325f9 on vladvildanov:vv-pipeline-on-connection-commands
into 4b28ab1 on predis:main.

@vladvildanov vladvildanov requested a review from chayim January 26, 2024 11:03
@vladvildanov vladvildanov changed the title Added pipelining for on-connection commands Improved connection handshake session Feb 21, 2024
@vladvildanov vladvildanov force-pushed the vv-pipeline-on-connection-commands branch from 86a541b to 60f778c Compare February 21, 2024 11:31
@vladvildanov vladvildanov force-pushed the vv-pipeline-on-connection-commands branch from 60f778c to 10bb01b Compare February 21, 2024 11:35
@vladvildanov
Copy link
Contributor Author

@tillkruss Review required

@vladvildanov
Copy link
Contributor Author

@tillkruss This PR is blocked by #1437 so it doesn't look like a total mess

@vladvildanov vladvildanov marked this pull request as draft February 22, 2024 08:01
@vladvildanov vladvildanov marked this pull request as ready for review February 22, 2024 08:01
@tillkruss
Copy link
Member

Can we rebase after merging the styling changes first? Hard to read otherwise.

@vladvildanov
Copy link
Contributor Author

@tillkruss Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants