[ISSUE-3402]: Ring.Pipelined return dial timeout error #3403
Conversation
|
@smnvdev thank you for your contribution, I hope @monkey92t or @vmihailenco can review this, If not we will review it when possible, but Ring is not our main priority at the moment, just letting you know. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the dial timeout error issue for the Ring.Pipelined method by adding a new test case and adjusting the pipeline processing logic.
- Adds a test in ring_test.go to ensure a dial timeout error is returned when an invalid address is provided with a low dial timeout.
- Modifies the pipeline processing in ring.go to correctly handle transactional pipelines using a distinct hook assignment.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ring_test.go | New test case to check dial timeout errors in the pipeline function. |
| ring.go | Updates to pipeline processing including refined hook assignment. |
Comments suppressed due to low confidence (1)
ring_test.go:293
- [nitpick] Consider enhancing the test by asserting that the error is specifically a dial timeout error rather than any error, to ensure the test accurately verifies the intended behavior.
Expect(err).To(HaveOccurred())
| _ = shard.Client.processTxPipelineHook(ctx, cmds) | ||
| } else { | ||
| _ = shard.Client.processPipelineHook(ctx, cmds) | ||
| cmds, hook = wrapMultiExec(ctx, cmds), shard.Client.processTxPipelineHook |
There was a problem hiding this comment.
[nitpick] The inline reassignment of both cmds and hook for the transaction pipeline branch might benefit from clearer separation or additional comments to improve code clarity between transactional and non-transactional pipelines.
| cmds, hook = wrapMultiExec(ctx, cmds), shard.Client.processTxPipelineHook | |
| // Modify cmds for transactional pipeline. | |
| cmds = wrapMultiExec(ctx, cmds) | |
| // Use the transactional pipeline hook. | |
| hook = shard.Client.processTxPipelineHook |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@smnvdev would you mind addressing this comment. Other than that I think it looks good and we can merge it.
There was a problem hiding this comment.
I agree it's a bit dense, but since both cmds and hook are tightly coupled here and modified only in the transactional branch, I thought keeping it compact improves local clarity. Happy to split if you prefer.
#3402