Skip to content

Conversation

@jamesx-improving
Copy link
Collaborator

@jamesx-improving jamesx-improving commented Dec 24, 2025

Issue link

This Pull Request is linked to issue (URL): #5113

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.

@jamesx-improving
Copy link
Collaborator Author

Redis-rs CI will be green once we merged #5101 and rebase.

@jamesx-improving jamesx-improving marked this pull request as ready for review December 25, 2025 01:02
@jamesx-improving jamesx-improving requested a review from a team as a code owner December 25, 2025 01:02
Copy link
Collaborator

@shohamazon shohamazon left a comment

Choose a reason for hiding this comment

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

reviewed the core :)

Copy link
Collaborator

@jduo jduo left a comment

Choose a reason for hiding this comment

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

Let's make this field a bool rather than an Option

@jamesx-improving jamesx-improving force-pushed the jamesx/add-tcp-no-delay-option branch from d0eeb8d to 6e071c4 Compare December 27, 2025 04:05
@jamesx-improving
Copy link
Collaborator Author

@shohamazon @jduo thank you for your reviews.

On the decision to make it optional, it is a backwards compatibility consideration, so that if other external binding upgrade to this version of glide core, but hasn't added the TCPNoDelay option, they can still work.

Will move the default to true logic to the rust side FFI boundary, so that all rust definition can be simplified as bool, but we maintain that backwards compatibility with other language bindings with no such option yet.

Please comment on this approach.

Copy link
Collaborator

@jduo jduo left a comment

Choose a reason for hiding this comment

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

One suggestion - let's describe in README.md this new option, its default, and when users should change it.

@jamesx-improving
Copy link
Collaborator Author

One suggestion - let's describe in README.md this new option, its default, and when users should change it.

Talked to @Aryex and apparently we are currently generating documentation from comments in code. Given we already have detailed comments in code for all 4 bindings (Go, Java, Node and Python), I'd say we are good.

Signed-off-by: James Xin <[email protected]>
Signed-off-by: James Xin <[email protected]>
Signed-off-by: James Xin <[email protected]>
Signed-off-by: James Xin <[email protected]>
Signed-off-by: James Xin <[email protected]>
Signed-off-by: James Xin <[email protected]>
@jamesx-improving jamesx-improving force-pushed the jamesx/add-tcp-no-delay-option branch from 73d4c22 to e127f13 Compare January 6, 2026 15:31
@jamesx-improving jamesx-improving merged commit 7aef569 into main Jan 6, 2026
69 of 73 checks passed
@jamesx-improving jamesx-improving deleted the jamesx/add-tcp-no-delay-option branch January 6, 2026 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants